NativeScript / NativeScript/android

Recursion in Util.java. I suspect that this will lead to issues passing a Google Tier2 CASA

Đang mở
#1,785 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Ngôn ngữ chính
C++
Star
563
Fork
144
Merge trung bình
10 giờ 46 phút
Pull request đã merge (30 ngày)
14

Mô tả

Environment
Provide version numbers for the following components (information can be retrieved by running tns info in your project folder or by inspecting the package.json of the project):

✔ Getting NativeScript components versions information...
✔ Component nativescript has 8.5.3 version and is up to date.
✔ Component @nativescript/core has 8.5.9 version and is up to date.
✔ Component @nativescript/ios has 8.5.2 version and is up to date.
✔ Component @nativescript/android has 8.5.2 version and is up to date.

Describe the bug
Hello together, I am currently preparing for a Tier2 CASA from Google. As I do have to provide Source Code scans using FluidAttacks and it detected a vulnerability in this context of printStackTrace I found the following issue.

https://github.com/NativeScript/android/blob/57388331bf85473d6c3cf5f115dd9d290b80d59b/test-app/app/src/main/java/com/tns/Util.java#L28

public static boolean isDebuggableApp(Context context) {
        int flags;
        try {
            flags = context.getPackageManager().getPackageInfo(context.getPackageName(), 0).applicationInfo.flags;
        } catch (NameNotFoundException e) {
            flags = 0;
            if (Util.isDebuggableApp(context)) {
                e.printStackTrace();
            }
        }

        boolean isDebuggableApp = ((flags & ApplicationInfo.FLAG_DEBUGGABLE) != 0);
        return isDebuggableApp;
    }

It is clear that printStackTrace() is only called in debug mode. However, I consider telling the guys at Google that this is save code as no good idea as it results in a recursion in the catch block. I would suspect that the e.printStackTrace(); in the catch block is actually never called, hence would it not make sense to completly remove it? I have also seen that the runtime provides
a check for isDebuggableApp. Maybe a save alternative would be to just call com.tns.Runtime.isDebuggable() ?

https://github.com/NativeScript/android/blob/57388331bf85473d6c3cf5f115dd9d290b80d59b/test-app/runtime/src/main/java/com/tns/Runtime.java#L248

I do not know the inner workings good enough to know if the runtime always exists at that point. Please consider it as just an idea.

Expected behavior
Catch should not run into a rekursion

Additional context

https://gitlab.com/fluidattacks/universe/-/issues/10406

Currently I am seeing lots of issues in FluidAttacks indirectly referencing this code. Below output is just an example. To my understanding this is a false positive as printStackTrace is not called for production builds. However, also in production builds the recursion would exist to my understanding.

234. Technical information leak - Stacktrace,CWE-209,The error stacktrace can be printed in OWASP/app/src/debug/java/com/tns/ErrorReport.java,CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:N/A:N/E:U/RL:U/RC:R,https://docs.fluidattacks.com/criteria/vulnerabilities/234,skims,SAST,92,"
   82 |         final int version = Build.VERSION.SDK_INT;
   83 |         if (version >= 23) {
   84 |             try {
   85 |                 // Necessary to work around compile errors with compileSdk 22 and lower
   86 |                 Method checkSelfPermissionMethod;
   87 |                 try {
   88 |                     checkSelfPermissionMethod = ActivityCompat.class.getMethod(""checkSelfPermission"", Context.class, Stri
   89 |                 } catch (NoSuchMethodException e) {
   90 |                     // method wasn't found, so there is no need to handle permissions explicitly
   91 |                     if (Util.isDebuggableApp(activity)) {
>  92 |                         e.printStackTrace();
   93 |                     }
   94 |                     return;
   95 |                 }
   96 |
   97 |                 int permission = (int) checkSelfPermissionMethod.invoke(null, activity, Manifest.permission.WRITE_EXTERNA
   98 |
   99 |                 if (permission != PackageManager.PERMISSION_GRANTED) {
  100 |                     // We don't have permission so prompt the user
  101 |                     Method requestPermissionsMethod = ActivityCompat.class.getMethod(""requestPermissions"", Activity.class
  102 |
      ^ Col 0
",java.java_info_leak_stacktrace

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu với khối catch của isDebuggableApp được tham chiếu trong test-app/app/src/main/java/com/tns/Util.java, sau đó kiểm tra điểm vào isDebuggable() trong test-app/runtime/src/main/java/com/tns/Runtime.java. Xác nhận rằng đường dẫn catch không thể đệ quy và kiểm tra để bảo đảm hành vi thu được đáp ứng hành vi mong đợi cũng như giải quyết mối lo ngại về quá trình quét đã được báo cáo.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
android, java
Lĩnh vực
mobile-dev, security
Loại issue
Lỗi
Độ khó
2/5
Thời gian dự kiến
1-3 giờ
Mức độ hoạt động
Đình trệ
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
35/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.