766397 - PasswordsRepoSession leaks PII (full record details!)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Description

[Nick Alexander :nalexander [he/him]]

D FxSync(31209)               PasswordsRepoSession :: Fetched record PasswordRecord {lastModified: 1340150783403, hostname: https://account.services.mozilla.com, formSubmitURL: https://account.services.mozilla.com, httpRealm: null, usernameField: username, passwordField: password, encryptedUsername: SNIP, encryptedPassword: SNIP, encType: 1, timeCreated: 1340144793600, timeLastUsed: 0, timePasswordChanged: 1340150783403, timesUsed: 0

(I removed the encryptedUsername and encryptedPassword).

Comment 1

[Matt Brubeck (:mbrubeck)]

Flagging this for security review.  Is this critical enough that we should respin the Fennec 14.0 release to fix it?

Comment 2

[Joe Drew (not getting mail)]

This is only in logcat, correct? I suspect we do not need to respin for this, though I'd definitely take it as a ride-along.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Joe Drew (:JOEDREW!) from comment #2)
> This is only in logcat, correct?

Right.

> I suspect we do not need to respin for
> this, though I'd definitely take it as a ride-along.

I agree.  The main risk here is that the log is exposed to other apps that might leak the information, maliciously or accidentally.  This is already mitigated by Android, because an app is not allowed to read the log unless the user grants it READ_LOGS permission at installation time.

Comment 4

[Tracy Walker [:tracy]]

Seems like a regression since the time
Bug 744794 - Remove password debugging information in logcat from release builds 
was fixed?

Also, for reference regarding password logging on desktop side:
Per bugs 
Bug 435624 - Passwords must be hided in debug log of password synchronisation
and
Bug 587072 - Password not encrypted in activity log
Trace level logging was allowed

Comment 5

[Richard Newman [:rnewman]]

(In reply to Tracy Walker [:tracy] from comment #4)
> Seems like a regression since the time
> Bug 744794 - Remove password debugging information in logcat from release
> builds 
> was fixed?

Different logging; that bug was dumping DB contents, not logging incoming records.

Comment 6

[Johnathan Nightingale [:johnath]]

rnewman added "has reviewed patch" here, but I see no patch that we can approve for landing. Github, I presume?

Comment 7

[Mike Connor [:mconnor]]

Yes, Nick fixed this in Github, will add link to cset when I'm not mobile.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

(In reply to Johnathan Nightingale [:johnath] from comment #6)
> rnewman added "has reviewed patch" here, but I see no patch that we can
> approve for landing. Github, I presume?

Correct; was mobile. Nick added the pull request URL to the URL field:

https://github.com/mozilla-services/android-sync/pull/218

Here it is on top of inbound; should apply cleanly everywhere, because it's simple.

Comment 9

[Nick Alexander :nalexander [he/him]]

I would like to clarify two points in prep for triage:

1. In normal use, a single password's details would be leaked to the log only O(1) times, and probably only exactly 1 time.

(Details: Logging statement is in PasswordsRepositorySession.fetchAndCloseCursorDeleted, which is essentially only called by .fetchSince, which in normal use maintains a high-water timestamp, so we should only see a local record as modified once.)

2. Log level was DEBUG, which is not logged by default.  By "logged" I mean: Android Sync does not write the message to the log at all unless the user has manually set the FxSync log tag to either DEBUG or VERBOSE.  So it's not possible for aLogcat (or another program with log permissions) to see the message without user action (or some other action that requires permissions).

Comment 10

[David Chan [:dchan]]

I talked it over with the rest of the security team and we are okay with no respin due to the following mitigating circumstances

1. As Nick mentioned, this is being logged at the DEBUG level. DEBUG logs aren't stored by default, though the code is still run
2. The default logging level is INFO [1]
3. Changing the logging level requires user interaction, e.g. through
adb shell setprop
4. Changing the property programmatically using System.setProperty() only exposes it to the current app / process. [2]


[1] - http://developer.android.com/reference/android/util/Log.html#isLoggable%28java.lang.String,%20int%29
[2] - http://groups.google.com/group/android-developers/browse_thread/thread/2d75bd8c637a4245

Comment 11

[Richard Newman [:rnewman]]

(In reply to David Chan [:dchan] from comment #10)
> I talked it over with the rest of the security team and we are okay with no
> respin due to the following mitigating circumstances
> 
> 1. As Nick mentioned, this is being logged at the DEBUG level. DEBUG logs
> aren't stored by default, though the code is still run

Strictly speaking, the generated string will exist only transiently in memory; our logging layer won't even call into android.util.Log unless the sync begins after FxSync's log level is set to DEBUG or TRACE.

(Hotspotting might even eliminate that code, and the string will never be computed at all.)

Comment 12

[Johnathan Nightingale [:johnath]]

.N+ given the added detail (let's still not let it linger) and we'd welcome a beta approval nom to go with the aurora nom just as soon as you're ready.

Comment 13

[Nick Alexander :nalexander [he/him]]

I've commited this to m-i; not sure what the lifting story is.

https://hg.mozilla.org/integration/mozilla-inbound/rev/542a27f0c445

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 634919 [details] [diff] [review]
Proposed patch. v1

Matching noms.

This is very low risk, so I'll land as soon as y'all are comfortable enough to +.

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/542a27f0c445

Comment 16

[Johnathan Nightingale [:johnath]]

Doo eet. (mozilla-beta tip only, please, not the relbranches)

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9101b13be64
https://hg.mozilla.org/releases/mozilla-beta/rev/0a5054b472f3

(I'm assuming it's correct to set status-firefox14 = fixed in this case; if not, please return to affected.)

Comment 18

[Aaron Train [:aaronmt]]

Verified Fixed via 14.0b10-candidate build #2, 20120628102429 through setprop INFO<->DEBUG priority constant