Closed Bug 741872 Opened 10 years ago Closed 6 years ago

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

Categories

(mozilla.org :: Security Assurance, task)

x86
macOS
task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: curtisk, Assigned: pauljt)

References

()

Details

(Whiteboard: [start 04/18/2012][target 04/25/2012])

audit sqlite interface to ensure parameterized queries and other SQLi defenses (I guess this is just the Fennec version?).
Whiteboard: [start 04/18/2012][target 04/25/2012]
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
(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
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.
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.