741872 - [Security Review][Action Item]WebSMS - sqlite queries

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance, task)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

audit sqlite interface to ensure parameterized queries and other SQLi defenses (I guess this is just the Fennec version?).

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Not sure where the sqlite information came from , but AFAICT sms on android uses  a ContentProvider to store and retrieve sms messages.

Data is added using the .insert() method, which isn't string based, so don't think there is an injection risk there.

Retrieval use query, which is good, but it is still possible to have injections in selection lists. This code worries me:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoSmsManager.java#775

Especially this line, since mNumbers is a String Array, maybe an injection risk (i don't really know enough about how content providers work)? 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoSmsManager.java#787

Not sure where mNumbers comes from - my guess would be be a user choosing a number from the interface, rather than simply typing one in. Even if it was user input, I expect it would come from the user, not from an sms message, so remote exploitation risk I think is low.

Mounir, some questions:

1- Can you confirm that SQLlite is not used on android (just making sure I didn't miss it)
2 - Do you see any security issues with the code at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoSmsManager.java#775

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Paul Theriault [:pauljt] from comment #1)
> Mounir, some questions:
> 
> 1- Can you confirm that SQLlite is not used on android (just making sure I
> didn't miss it)

AFAIK, ContentProvider is a a wrapper around a SQLite database, see [1].

> 2 - Do you see any security issues with the code at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoSmsManager.java#775

selectionArgs is the recommended way to prevent SQL injection [1] so, yes, that's why I put that comment. Note that mNumbers comes from the content where a web developer can create a SmsFilter object with a list of numbers to filter against.

[1] 
http://developer.android.com/guide/topics/providers/content-provider-basics.html

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ok thanks Mounir, I wasn't sure about what was behind the Content Provider code.

I'll guess I will just leave this open then until that gets patched. I was thinking there might be mitigating controls, like maybe only the sms database might be affected, and the component would have full access to the sms database anyways. But its probably easier just to fix it and remove the risk. (And I suppose there may be a use case where an application might have access to read sms but not modify the database.

Comment 4

[Mounir Lamouri (:mounir)]

Actually, consumer of the API are allowed to read SMS or delete them but they can't modify them so we clearly don't want an app to have full access to the database.

IIRC, there is no bug open on that issue.