Potential vulnerability detected into a security audit.

[Hanzofm]

Bug Report

Problem

Recently we have submitted our Ionic App for a security audit and this is report about Cordova-

Android:
-Vulnerability: Code Correctness: Double-Checked Locking
-Description: The method relies on double-checked locking, an incorrect idiom that does not achieve the intended effect.
-Fortify Priority: High.
-Fortify Priority: Explotable.

-Sink:

File Path : File Path : project/node_modules/cordova-android/framework/src/org/apache/cordova/NativeToJsMessageQueue.java Line Number : 93
if (newMode != null) {
                        newMode.reset();
                        if (!paused && !queue.isEmpty()) {
                            newMode.onNativeToJsMessageAvailable(this);
                        }

Recommendation:
Avoid double null-checking adding into code complexity and maintenance burden. Synchronize and then null check

Environment
Ionic info:

Ionic:

   ionic (Ionic CLI)  : 4.8.0
   Ionic Framework    : ionic-angular 3.9.2
   @ionic/app-scripts : 3.2.1

Cordova:

   cordova (Cordova CLI) : 8.0.0
   Cordova Platforms     : android 7.1.4, ios 4.5.5
   Cordova Plugins       : no whitelisted plugins (14 plugins total)

System:

   Android SDK Tools : 26.1.1 (/Users/user/Library/Android/sdk)
   ios-deploy        : 1.9.2
   ios-sim           : 7.0.0
   NodeJS            : v9.9.0 (/usr/local/bin/node)
   npm               : 5.7.1
   OS                : macOS Mojave
   Xcode             : Xcode 10.2 Build version 10E125

The version of Cordova-android is 7.1.4 but into 8.0.0 version the problem persist

It should be fixed?

[janpio]

Can you explain the problem a bit more?

[Hanzofm]

Hi, really I dont think this really be a problem.

I think that it refers to(but i'm not sure):

if(newMode != null)

I have founded this on wiki
https://en.wikipedia.org/wiki/Double-checked_locking
but I was hoping that you could help me a little more :p

The problem is a bit frustrating because if not solve this "security issue" we cant publish the app

Thanks a lot

[janpio]

Well, whoever did the audit and blocks the release sure can tell you what this all means, can't they?

[Hanzofm]

Thanks. I am gonna ask for more details to the auditing company. Back soon

[Hanzofm]

After read some documentation of "Double check" problem I would like to share my conclusions and see if it if it would be fixed on the platform.

Related with the Wiki page of the known code mistake, in the Java section this is a similar example of a reported code by audit:

class Foo {
    private Helper helper;
    public Helper getHelper() {
        if (helper == null) {
            synchronized (this) {
                if (helper == null) {
                    helper = new Helper();
                }
            }
        }
        return helper;
    }

    // other functions and members...
}

The problem is about concurrency between threads in extremely cases, copy-paste:

For example, consider the following sequence of events:

1.Thread A notices that the value is not initialized, so it obtains the lock and begins to initialize the value.
2.Due to the semantics of some programming languages, the code generated by the compiler is allowed to update the shared variable to point to a partially constructed object before A has finished performing the initialization. For example, in Java if a call to a constructor has been inlined then the shared variable may immediately be updated once the storage has been allocated but before the inlined constructor initializes the object.[6]
3.Thread B notices that the shared variable has been initialized (or so it appears), and returns its value. Because thread B believes the value is already initialized, it does not acquire the lock. If B uses the object before all of the initialization done by A is seen by B (either because A has not finished initializing it or because some of the initialized values in the object have not yet percolated to the memory B uses (cache coherence)), the program will likely crash.

In relation to your code it would be similar with:

public class NativeToJsMessageQueue {
...
private BridgeMode activeBridgeMode; /**/ helper in the example**

...

 public void setBridgeMode(int value) {
        if (value < -1 || value >= bridgeModes.size()) {
            LOG.d(LOG_TAG, "Invalid NativeToJsBridgeMode: " + value);
        } else {
            BridgeMode newMode = value < 0 ? null : bridgeModes.get(value);
            if (newMode != activeBridgeMode) {
                LOG.d(LOG_TAG, "Set native->JS mode to " + (newMode == null ? "null" : newMode.getClass().getSimpleName()));
                synchronized (this) {
                    activeBridgeMode = newMode; // inline asignation
                    if (newMode != null) { // double check problem
                        newMode.reset();
                        if (!paused && !queue.isEmpty()) {
                            newMode.onNativeToJsMessageAvailable(this);
                        }
                    }
                }
            }
        }
    } 

...
}

According to the Wiki article the problem can be avoided on Java versions >1.5 with the volatile keyword like this example:

class Foo {
    private volatile Helper helper;
    public Helper getHelper() {
        Helper localRef = helper;
        if (localRef == null) {
            synchronized (this) {
                localRef = helper;
                if (localRef == null) {
                    helper = localRef = new Helper();
                }
            }
        }
        return localRef;
    }

    // other functions and members...
}

Note the local variable "localRef", which seems unnecessary. The effect of this is that in cases where helper is already initialized (i.e., most of the time), the volatile field is only accessed once (due to "return localRef;" instead of "return helper;"), which can improve the method's overall performance by as much as 25 percent

Do you think it would be possible for you to implement it in the repository?

More info about this problem by OWASP

Thanks a lot

[Hanzofm]

Hi, some news about this?

[janpio]

I still don't understand what the actual problem is here, besides that someone tells you there is a problem. Sorry.

Feel free to open a Pull Request suggesting some code changes that fix the problem, you seems to have a pretty good understanding what the problem is now. Maybe I can understand it when I see the code that has to be changed.

[brody4hire]

I would like to add a few more comments:

A link to the code under discussion, which is in the setBridgeMode member function, would have been nice. Here it is: https://github.com/apache/cordova-android/blob/8.0.0/framework/src/org/apache/cordova/NativeToJsMessageQueue.java#L82-L100

This link is tied to the most recent release tag (8.0.0) and shows the entire function block for the sake of context. And here is a link to the declaration of bridgeModes: https://github.com/apache/cordova-android/blob/8.0.0/framework/src/org/apache/cordova/NativeToJsMessageQueue.java#L55-L58

I would consider the title to be a bit misleading. I cannot see how this could be a real security vulnerability at this point. I think it is also a bit unfortunate that the external tool labels this as a vulnerability with a priority level of "high". I think calling this a "potential vunerability" would have been more appropriate.

Looking at the code, I think it does not match the double-checked locking anti-pattern in Java ([1]) for the following reasons:

• newMode is a local variable (I think that it is derived from an incoming argument or is used to set a non-local variable does not matter here)
• we are not changing its value

I do think it should be possible to make some improvements related to this code, such as:

• move the synchronized part into a separate private synchronized function
• declare activeBridgeMode as @NonNull and use a special noop bridge in case no native bridge is wanted

[1] https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java

[Hanzofm]

I would consider the title to be a bit misleading. I cannot see how this could be a real security vulnerability at this point. I think it is also a bit unfortunate that the external tool labels this as a vulnerability with a priority level of "high". I think calling this a "potential vunerability" would have been more appropriate.

Sorry for the title, I decide to set it due to the report of the auditing company marked this at a "high vulnerability" I will change the title to more appropiate description.

Looking at the code, I think it does not match the double-checked locking anti-pattern in Java ([1]) for the following reasons:

• newMode is a local variable (I think that it is derived from an incoming argument or is used to set a non-local variable does not matter here)
• we are not changing its value

In my opinion the main problem with multithreading and the Double-checked-locking is in our case the variable activeBridgeMode that is a global class variable shared into different threads.

In the code on line 86 defines a helper local variable called newMode wich is defined with the value of activeBridgeMode that is shared class member.

Double null check is checked with this class variable value and then inside de lock is checked again. I think that would be the reported problem by the auditing company.

In the Wiki example code:

class Foo {
    private volatile Helper helper;
    public Helper getHelper() {
        Helper localRef = helper;
        if (localRef == null) {
            synchronized (this) {
                localRef = helper;
                if (localRef == null) {
                    helper = localRef = new Helper();
                }
            }
        }
        return localRef;
    }

    // other functions and members...
}

The example is similar. There are a local variable wich their value is defined by the global shared variable.

I think that the problem would be solved by adding the Volatile decorator to the shared variable. Bu I am not sure.

Thanks for your time

[Hanzofm]

Hi to everyone.

It is planned to make some progress on this issue?

Thanks for your time

[brody4hire]

I don't have so much time to look at this right now and I think the other maintainers are pretty overloaded as well. I suggest you try out some of your own ideas to solve your own problem and consider raising a PR, if appropriate. And feel free to make your own fork according to the Apache 2.0 license, if you like.