Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2802603002:
getInstalledRelatedApps: Introduce random delay to stop timing attacks. (Closed)
|
Created:
3 years, 8 months ago by Matt Giuca Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, darin-cc_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptiongetInstalledRelatedApps: Introduce random delay to stop timing attacks.
Queries to this API have a sleep of 0--8ms, randomly chosen based on a
SHA-256 hash of the app ID, salted with a unique stable device ID. This
prevents websites from being able to determine whether a non-related app
is installed based on the time the query takes.
The website is unable to compare the timing of different apps because
they have different random delays, and unable to predict the random
delay for a given device, because the salt is unknown.
BUG=707071
Review-Url: https://codereview.chromium.org/2802603002
Cr-Commit-Position: refs/heads/master@{#464322}
Committed: https://chromium.googlesource.com/chromium/src/+/245e999d5e19bd5f4bd44feed0336b2156581df1
Patch Set 1 #Patch Set 2 : Reset the salt every time the browser process starts. #Patch Set 3 : Added tests for PackageHash and the delay in InstalledAppProviderImpl. #
Total comments: 8
Patch Set 4 : Rebase. #Patch Set 5 : Update comment. #Patch Set 6 : Use system random source, and HMAC, instead of UUID and concatenation. #
Total comments: 5
Patch Set 7 : Reword comments. #Patch Set 8 : Base off CL 2813153002. #Patch Set 9 : Test: Update comment to refer to HMAC algorithm. #Patch Set 10 : Use Handler.postDelay instead of Thread.sleep, to avoid choking any thread. #
Total comments: 8
Patch Set 11 : Respond to reviews. #Patch Set 12 : Rebase. #
Depends on Patchset: Messages
Total messages: 41 (17 generated)
Description was changed from ========== getInstalledRelatedApps: Introduce random delay to stop timing attacks. Queries to this API have a sleep of 0--8ms, randomly chosen based on a SHA-256 hash of the app ID, salted with a unique stable device ID. This prevents websites from being able to determine whether a non-related app is installed based on the time the query takes. The website is unable to compare the timing of different apps because they have different random delays, and unable to predict the random delay for a given device, because the salt is unknown. BUG=707071 ========== to ========== getInstalledRelatedApps: Introduce random delay to stop timing attacks. Queries to this API have a sleep of 0--8ms, randomly chosen based on a SHA-256 hash of the app ID, salted with a unique stable device ID. This prevents websites from being able to determine whether a non-related app is installed based on the time the query takes. The website is unable to compare the timing of different apps because they have different random delays, and unable to predict the random delay for a given device, because the salt is unknown. BUG=707071 ==========
Patchset #3 (id:40001) has been deleted
mgiuca@chromium.org changed reviewers: + palmer@chromium.org
Hi Chris, Please have a look and see if this implementation makes sense to you from a security perspective. If good, I will send on to an Android reviewer. Thanks Matt
https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:124: // The time delay is the low 13 bits of the hash in usec (between 0 and 8.192ms). Pico-nit: Use "ms" consistently (not "usec"). https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:40: String salt = getGlobalSalt(); Instead, use ByteArrayGenerator.getBytes (content/public/android/java/src/org/chromium/content/browser/crypto/ByteArrayGenerator.java) https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:41: String input = salt + ':' + packageName; It not hugely important for this low-entropy, best-effort application, but FYI: This pattern has a bad smell to crypto people, because of https://en.wikipedia.org/wiki/Length_extension_attack. The standard thing to do here is byte[] salt = getRandomBytes(32), then use HMAC-SHA256 with that salt as the HMAC 'key'. See javax.crypto.Mac and javax.crypto.spec.SecretKeySpec. Here's an example: https://www.jokecamp.com/blog/examples-of-creating-base64-hashes-using-hmac-s...
Thanks for reviewing this. I ran out of time to work on this today, but sending this to get a better understanding on the first comment. https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:124: // The time delay is the low 13 bits of the hash in usec (between 0 and 8.192ms). On 2017/04/07 18:56:33, palmer wrote: > Pico-nit: Use "ms" consistently (not "usec"). Not sure what you mean by this. Are you asking me to change the logic or just the comment? (The logic is the low 13 bits of the hash represent the time delay in μs, not ms.) I've updated the comment to "μs" not "usec". But the variable names remain as "Usec" because I can't use Unicode in a variable name and "Us" is ambiguous. https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:40: String salt = getGlobalSalt(); On 2017/04/07 18:56:33, palmer wrote: > Instead, use ByteArrayGenerator.getBytes > (content/public/android/java/src/org/chromium/content/browser/crypto/ByteArrayGenerator.java) Acknowledged. Will review tomorrow (ran out of time). https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:41: String input = salt + ':' + packageName; On 2017/04/07 18:56:33, palmer wrote: > It not hugely important for this low-entropy, best-effort application, but FYI: > This pattern has a bad smell to crypto people, because of > https://en.wikipedia.org/wiki/Length_extension_attack. > > The standard thing to do here is byte[] salt = getRandomBytes(32), then use > HMAC-SHA256 with that salt as the HMAC 'key'. See javax.crypto.Mac and > javax.crypto.spec.SecretKeySpec. Here's an example: > https://www.jokecamp.com/blog/examples-of-creating-base64-hashes-using-hmac-s... Acknowledged. Will review tomorrow.
> > Pico-nit: Use "ms" consistently (not "usec"). > > Not sure what you mean by this. Are you asking me to change the logic or just the comment? > > (The logic is the low 13 bits of the hash represent the time delay in μs, not ms.) I've updated the comment to "μs" not "usec". But the variable names remain as "Usec" because I can't use Unicode in a variable name and "Us" is ambiguous. Oh, sorry. Yes, I meant the comment. Looks good now.
Patchset #4 (id:80001) has been deleted
Patchset #6 (id:130001) has been deleted
Patchset #6 (id:150001) has been deleted
mgiuca@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for Android reviews. I have a couple of questions (for both of you) in the comments. Thanks https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:40: String salt = getGlobalSalt(); On 2017/04/10 08:33:40, Matt Giuca wrote: > On 2017/04/07 18:56:33, palmer wrote: > > Instead, use ByteArrayGenerator.getBytes > > > (content/public/android/java/src/org/chromium/content/browser/crypto/ByteArrayGenerator.java) > > Acknowledged. Will review tomorrow (ran out of time). Done. https://codereview.chromium.org/2802603002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:41: String input = salt + ':' + packageName; On 2017/04/10 08:33:40, Matt Giuca wrote: > On 2017/04/07 18:56:33, palmer wrote: > > It not hugely important for this low-entropy, best-effort application, but > FYI: > > This pattern has a bad smell to crypto people, because of > > https://en.wikipedia.org/wiki/Length_extension_attack. > > > > The standard thing to do here is byte[] salt = getRandomBytes(32), then use > > HMAC-SHA256 with that salt as the HMAC 'key'. See javax.crypto.Mac and > > javax.crypto.spec.SecretKeySpec. Here's an example: > > > https://www.jokecamp.com/blog/examples-of-creating-base64-hashes-using-hmac-s... > > Acknowledged. Will review tomorrow. Done. https://codereview.chromium.org/2802603002/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:272: Thread.sleep(millis, nanos); tedchoc: I think this Thread.sleep might be bad (if I set this to a huge number, it blocks the main browser UI). It's not terrible, since it's designed to sleep on the order of 0-8ms which is about how long the computation will take anyway. But it would be better if we can post a delayed task instead of just sleeping the thread. I am not familiar with the JS side of things (in C++ it would be a PostDelayedTask). Can you recommend how to do it? Or is the Thread.sleep OK? https://codereview.chromium.org/2802603002/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:86: // TODO(mgiuca): How to handle this? DO NOT SUBMIT. palmer: Do you have any suggestion of what to do here? This can fail if the file /dev/urandom is not found (IOException) or if it returns less bytes than desired (GeneralSecurityException), neither of which *should* happen on a Linux system. Is it OK to crash the browser in this case? Otherwise I think we have to propagate an error all the way up to JavaScript (to reject the promise). Why do we have to use /dev/urandom, instead of Java's internal crypto library which presumably uses /dev/urandom internally? I could use javax.crypto.KeyGenerator which "will get [bytes] using the SecureRandom implementation of the highest-priority installed provider as the source of randomness. (If none of the installed providers supply an implementation of SecureRandom, a system-provided source of randomness will be used.)" That has no such IO errors to handle.
https://codereview.chromium.org/2802603002/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:272: Thread.sleep(millis, nanos); On 2017/04/11 08:34:08, Matt Giuca wrote: > tedchoc: I think this Thread.sleep might be bad (if I set this to a huge number, > it blocks the main browser UI). > > It's not terrible, since it's designed to sleep on the order of 0-8ms which is > about how long the computation will take anyway. But it would be better if we > can post a delayed task instead of just sleeping the thread. > > I am not familiar with the JS side of things (in C++ it would be a > PostDelayedTask). Can you recommend how to do it? Or is the Thread.sleep OK? We definitely shouldn't sleep the UI thread. That will cause a StrictMode violation (a red flash showing we're doing bad things). Can this call be made async? If you were do do something like: private [void] isAppInstalledAndAssociatedWithOrigin( String packageName, URI frameUrl, PackageManager pm, [final Callback<Boolean> callback]) { new Handler().postDelayed(new Runnable() { @Override public void run() { callback.call(helperMethodThatDoesStuffForRealz(packageName, frameUrl, pm); } }, <random delay>); } FWIW 8ms is half a render frame, so that is actually not something we should do lightly.
> palmer: Do you have any suggestion of what to do here? > > This can fail if the file /dev/urandom is not found (IOException) or if it returns less bytes than desired (GeneralSecurityException), neither of which *should* happen on a Linux system. Is it OK to crash the browser in this case? Otherwise I think we have to propagate an error all the way up to JavaScript (to reject the promise). Yes, I would crash the browser. If /dev/urandom is not available or does not return enough bytes, then the entire operating system is on fire and we're all going to die anyway. It backs everything else in browser cryptography (e.g. TLS), so if it's not working, we must not continue browsing the web.:) > Why do we have to use /dev/urandom, instead of Java's internal crypto library which presumably uses /dev/urandom internally? I could use javax.crypto.KeyGenerator which "will get [bytes] using the SecureRandom implementation of the highest-priority installed provider as the source of randomness. (If none of the installed providers supply an implementation of SecureRandom, a system-provided source of randomness will be used.)" That has no such IO errors to handle. Java implementations have a long history of extremely creepy, weird bugs in the 'secure' random generator API: falling back to predictable RNGs, for example. I wrote the /dev/urandom code specifically to fix such a bug. Additionally, the Java APIs are absurdly complex (which is how the bugs creep in), when in reality all you need to do is read from a file. So I would just use that API, and if it raises any exception, die immediately.
On 2017/04/11 18:22:43, palmer wrote: > > palmer: Do you have any suggestion of what to do here? > > > > This can fail if the file /dev/urandom is not found (IOException) or if it > returns less bytes than desired (GeneralSecurityException), neither of which > *should* happen on a Linux system. Is it OK to crash the browser in this case? > Otherwise I think we have to propagate an error all the way up to JavaScript (to > reject the promise). > > Yes, I would crash the browser. If /dev/urandom is not available or does not > return enough bytes, then the entire operating system is on fire and we're all > going to die anyway. It backs everything else in browser cryptography (e.g. > TLS), so if it's not working, we must not continue browsing the web.:) > > > Why do we have to use /dev/urandom, instead of Java's internal crypto library > which presumably uses /dev/urandom internally? I could use > javax.crypto.KeyGenerator which "will get [bytes] using the SecureRandom > implementation of the highest-priority installed provider as the source of > randomness. (If none of the installed providers supply an implementation of > SecureRandom, a system-provided source of randomness will be used.)" That has no > such IO errors to handle. > > Java implementations have a long history of extremely creepy, weird bugs in the > 'secure' random generator API: falling back to predictable RNGs, for example. I > wrote the /dev/urandom code specifically to fix such a bug. Additionally, the > Java APIs are absurdly complex (which is how the bugs creep in), when in reality > all you need to do is read from a file. > > So I would just use that API, and if it raises any exception, die immediately. Thanks for the detailed explanation. https://codereview.chromium.org/2802603002/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:272: Thread.sleep(millis, nanos); On 2017/04/11 16:01:32, Ted C wrote: > On 2017/04/11 08:34:08, Matt Giuca wrote: > > tedchoc: I think this Thread.sleep might be bad (if I set this to a huge > number, > > it blocks the main browser UI). > > > > It's not terrible, since it's designed to sleep on the order of 0-8ms which is > > about how long the computation will take anyway. But it would be better if we > > can post a delayed task instead of just sleeping the thread. > > > > I am not familiar with the JS side of things (in C++ it would be a > > PostDelayedTask). Can you recommend how to do it? Or is the Thread.sleep OK? > > We definitely shouldn't sleep the UI thread. That will cause a StrictMode > violation (a red flash showing we're doing bad things). Oh yeah, I saw that red flash and didn't know what it was. > Can this call be made async? If you were do do something like: > > private [void] isAppInstalledAndAssociatedWithOrigin( > String packageName, URI frameUrl, PackageManager pm, [final > Callback<Boolean> callback]) { > new Handler().postDelayed(new Runnable() { > @Override > public void run() { > callback.call(helperMethodThatDoesStuffForRealz(packageName, > frameUrl, pm); > } > }, <random delay>); > } Thanks, that looks like exactly what I want. > FWIW 8ms is half a render frame, so that is actually not something we should do > lightly. Given that it can take ~8ms (or in extreme cases, double that) to actually process the JSON, is this something we also should be doing on another thread instead of doing this operation here on the UI thread? (Which is why I ask above about whether postDelayed is going to use the UI thread or a worker thread.) It looks like (from the Handler documentation) that the above code will still run that helperMethodThatDoesStuffForRealz code on the main thread, and I would have to do more work to farm it off to a separate thread.
On 2017/04/12 01:05:58, Matt Giuca wrote: > On 2017/04/11 18:22:43, palmer wrote: > > > palmer: Do you have any suggestion of what to do here? > > > > > > This can fail if the file /dev/urandom is not found (IOException) or if it > > returns less bytes than desired (GeneralSecurityException), neither of which > > *should* happen on a Linux system. Is it OK to crash the browser in this case? > > Otherwise I think we have to propagate an error all the way up to JavaScript > (to > > reject the promise). > > > > Yes, I would crash the browser. If /dev/urandom is not available or does not > > return enough bytes, then the entire operating system is on fire and we're all > > going to die anyway. It backs everything else in browser cryptography (e.g. > > TLS), so if it's not working, we must not continue browsing the web.:) > > > > > Why do we have to use /dev/urandom, instead of Java's internal crypto > library > > which presumably uses /dev/urandom internally? I could use > > javax.crypto.KeyGenerator which "will get [bytes] using the SecureRandom > > implementation of the highest-priority installed provider as the source of > > randomness. (If none of the installed providers supply an implementation of > > SecureRandom, a system-provided source of randomness will be used.)" That has > no > > such IO errors to handle. > > > > Java implementations have a long history of extremely creepy, weird bugs in > the > > 'secure' random generator API: falling back to predictable RNGs, for example. > I > > wrote the /dev/urandom code specifically to fix such a bug. Additionally, the > > Java APIs are absurdly complex (which is how the bugs creep in), when in > reality > > all you need to do is read from a file. > > > > So I would just use that API, and if it raises any exception, die immediately. > > Thanks for the detailed explanation. > > https://codereview.chromium.org/2802603002/diff/170001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java > (right): > > https://codereview.chromium.org/2802603002/diff/170001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:272: > Thread.sleep(millis, nanos); > On 2017/04/11 16:01:32, Ted C wrote: > > On 2017/04/11 08:34:08, Matt Giuca wrote: > > > tedchoc: I think this Thread.sleep might be bad (if I set this to a huge > > number, > > > it blocks the main browser UI). > > > > > > It's not terrible, since it's designed to sleep on the order of 0-8ms which > is > > > about how long the computation will take anyway. But it would be better if > we > > > can post a delayed task instead of just sleeping the thread. > > > > > > I am not familiar with the JS side of things (in C++ it would be a > > > PostDelayedTask). Can you recommend how to do it? Or is the Thread.sleep OK? > > > > We definitely shouldn't sleep the UI thread. That will cause a StrictMode > > violation (a red flash showing we're doing bad things). > > Oh yeah, I saw that red flash and didn't know what it was. > > > Can this call be made async? If you were do do something like: > > > > private [void] isAppInstalledAndAssociatedWithOrigin( > > String packageName, URI frameUrl, PackageManager pm, [final > > Callback<Boolean> callback]) { > > new Handler().postDelayed(new Runnable() { > > @Override > > public void run() { > > callback.call(helperMethodThatDoesStuffForRealz(packageName, > > frameUrl, pm); > > } > > }, <random delay>); > > } > > Thanks, that looks like exactly what I want. > > > FWIW 8ms is half a render frame, so that is actually not something we should > do > > lightly. > > Given that it can take ~8ms (or in extreme cases, double that) to actually > process the JSON, is this something we also should be doing on another thread > instead of doing this operation here on the UI thread? (Which is why I ask above > about whether postDelayed is going to use the UI thread or a worker thread.) > > It looks like (from the Handler documentation) that the above code will still > run that helperMethodThatDoesStuffForRealz code on the main thread, and I would > have to do more work to farm it off to a separate thread. Ah ha, take a look at AsyncTask then. You might want to still post the start of the task though to no block any thread. But if your background work takes time, won't that make the timing attack easy to spot where it takes 8sec to process and it was delayed 8sec. Is it better to try and always force the response to take constant (and slowest) time?
On 2017/04/12 01:25:40, Ted C wrote: > Ah ha, take a look at AsyncTask then. You might want to still post the start of > the task though to no block any thread. OK. > But if your background work takes time, won't that make the timing attack easy > to spot where it takes 8sec to process and it was delayed 8sec. Is it better to > try and always force the response to take constant (and slowest) time? We don't know what the "slowest" time is; if we do that we run the risk of not picking a long enough time, and then any work that takes longer than that time will be trivial to spot. Instead, we introduce a random offset (between 0 and 8ms). If the work takes ~the same or less time than the max offset we choose, the noise will drown out the signal. If the work happens to take significantly longer (e.g., double) than the max offset, then the signal will be visible in the noise, but it will be gradually easier to detect, rather than suddenly being exploitable if we go past a certain time. (That's my understanding of it; there's a lengthy discussion and timing analysis on the bug if you are interested in. We went around in circles on how to deal with this for a week, and this was the solution we came up with.)
On 2017/04/12 01:42:31, Matt Giuca wrote:
> On 2017/04/12 01:25:40, Ted C wrote:
> > Ah ha, take a look at AsyncTask then. You might want to still post the
start
> of
> > the task though to no block any thread.
>
> OK.
>
> > But if your background work takes time, won't that make the timing attack
easy
> > to spot where it takes 8sec to process and it was delayed 8sec. Is it
better
> to
> > try and always force the response to take constant (and slowest) time?
>
> We don't know what the "slowest" time is; if we do that we run the risk of not
> picking a long enough time, and then any work that takes longer than that time
> will be trivial to spot.
>
> Instead, we introduce a random offset (between 0 and 8ms). If the work takes
> ~the same or less time than the max offset we choose, the noise will drown out
> the signal. If the work happens to take significantly longer (e.g., double)
than
> the max offset, then the signal will be visible in the noise, but it will be
> gradually easier to detect, rather than suddenly being exploitable if we go
past
> a certain time.
>
> (That's my understanding of it; there's a lengthy discussion and timing
analysis
> on the bug if you are interested in. We went around in circles on how to deal
> with this for a week, and this was the solution we came up with.)
I guess I was thinking that you would guarantee a specific time and if it takes
longer than you just return the default no answer.
Hacky version of that:
public void doStuff(final Callback<Boolean> callback) {
final AtomicBoolean retVal = new AtomicBoolean();
new AsyncTask<Void, Void, Void>() {
void doInBackground() {
retVal.set(<outcome of logic>);
}
}.executeOnExcecutor(THREAD_POOL);
new Handler().postDelayed(new Runnable() {
void run() {
callback.call(retVal.get());
}
}, MAX_ALLOWED_TIME);
}
I was thinking that you would just guarantee a time. The problem would be that
if devices are always slower than that, it would take a cycle or more complex
code to bump up the threshold. Again, I am no security expert and likely saying
things that are just as flawed for other reasons.
Also the code is fraught with errors (syntax for sure and probably logical as
well) and things I wouldn't necessarily suggest doing in actual code.
On 2017/04/12 01:54:45, Ted C wrote: > I guess I was thinking that you would guarantee a specific time and if it takes > longer than you just return the default no answer. > > <snip> > > I was thinking that you would just guarantee a time. The problem would be that > if devices are always slower than that, it would take a cycle or more complex > code to bump up the threshold. Again, I am no security expert and likely saying > things that are just as flawed for other reasons. I think that would be fine from a security standpoint but it would make the results of the call flaky; if you have a slow device or something going on in the background, it may simply never work, depending on the timer we set. I'd prefer to keep the current approach and make the result deterministic. Thanks for the suggestion about AsyncTask. This API very neatly encapsulated the problem of moving that off to a separate thread. I've done that in a separate CL (since this is a pre-existing issue unrelated to the timing delay): https://codereview.chromium.org/2813153002/ Instead of using Handler.postDelayed, I figured I could just call Thread.sleep() from inside the background thread. I tested it (with a 3 second sleep) and it does not block the UI thread, nor show a red flash. So I am happy with this approach now. PTAL. (Note: If possible I would like to be able to land this before branch point tomorrow :) Otherwise this will have to be merged or the feature disabled for a milestone.) Thanks in advance!
https://codereview.chromium.org/2802603002/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:86: // TODO(mgiuca): How to handle this? DO NOT SUBMIT. On 2017/04/11 08:34:08, Matt Giuca wrote: > palmer: Do you have any suggestion of what to do here? > > This can fail if the file /dev/urandom is not found (IOException) or if it > returns less bytes than desired (GeneralSecurityException), neither of which > *should* happen on a Linux system. Is it OK to crash the browser in this case? > Otherwise I think we have to propagate an error all the way up to JavaScript (to > reject the promise). > > Why do we have to use /dev/urandom, instead of Java's internal crypto library > which presumably uses /dev/urandom internally? I could use > javax.crypto.KeyGenerator which "will get [bytes] using the SecureRandom > implementation of the highest-priority installed provider as the source of > randomness. (If none of the installed providers supply an implementation of > SecureRandom, a system-provided source of randomness will be used.)" That has no > such IO errors to handle. Done (discussed in comment thread).
On 2017/04/12 05:37:54, Matt Giuca wrote: > On 2017/04/12 01:54:45, Ted C wrote: > > I guess I was thinking that you would guarantee a specific time and if it > takes > > longer than you just return the default no answer. > > > > <snip> > > > > I was thinking that you would just guarantee a time. The problem would be > that > > if devices are always slower than that, it would take a cycle or more complex > > code to bump up the threshold. Again, I am no security expert and likely > saying > > things that are just as flawed for other reasons. > > I think that would be fine from a security standpoint but it would make the > results of the call flaky; if you have a slow device or something going on in > the background, it may simply never work, depending on the timer we set. I'd > prefer to keep the current approach and make the result deterministic. > > Thanks for the suggestion about AsyncTask. This API very neatly encapsulated the > problem of moving that off to a separate thread. I've done that in a separate CL > (since this is a pre-existing issue unrelated to the timing delay): > https://codereview.chromium.org/2813153002/ > > Instead of using Handler.postDelayed, I figured I could just call Thread.sleep() > from inside the background thread. I tested it (with a 3 second sleep) and it > does not block the UI thread, nor show a red flash. So I am happy with this > approach now. PTAL. > > (Note: If possible I would like to be able to land this before branch point > tomorrow :) Otherwise this will have to be merged or the feature disabled for a > milestone.) Thanks in advance! The gotcha with AsyncTask is that it either is using a shared thread pool or is serial. By doing thread.sleep we are unnecessarily clogging up resources. I think to be the best citizen we should post the delay instead of sleeping.
On 2017/04/12 05:48:14, Ted C wrote: > On 2017/04/12 05:37:54, Matt Giuca wrote: > > On 2017/04/12 01:54:45, Ted C wrote: > > > I guess I was thinking that you would guarantee a specific time and if it > > takes > > > longer than you just return the default no answer. > > > > > > <snip> > > > > > > I was thinking that you would just guarantee a time. The problem would be > > that > > > if devices are always slower than that, it would take a cycle or more > complex > > > code to bump up the threshold. Again, I am no security expert and likely > > saying > > > things that are just as flawed for other reasons. > > > > I think that would be fine from a security standpoint but it would make the > > results of the call flaky; if you have a slow device or something going on in > > the background, it may simply never work, depending on the timer we set. I'd > > prefer to keep the current approach and make the result deterministic. > > > > Thanks for the suggestion about AsyncTask. This API very neatly encapsulated > the > > problem of moving that off to a separate thread. I've done that in a separate > CL > > (since this is a pre-existing issue unrelated to the timing delay): > > https://codereview.chromium.org/2813153002/ > > > > Instead of using Handler.postDelayed, I figured I could just call > Thread.sleep() > > from inside the background thread. I tested it (with a 3 second sleep) and it > > does not block the UI thread, nor show a red flash. So I am happy with this > > approach now. PTAL. > > > > (Note: If possible I would like to be able to land this before branch point > > tomorrow :) Otherwise this will have to be merged or the feature disabled for > a > > milestone.) Thanks in advance! > > The gotcha with AsyncTask is that it either is using a shared thread pool or is > serial. By doing thread.sleep we are unnecessarily clogging up resources. I > think to be the best citizen we should post the delay instead of sleeping. OK that's fair, but now that I am on a background thread, can I just do a simple delay on the same thread without having to figure out how to create a Handler on a different thread: new Handler().postDelayed(<call the callback>, delayMillis); ? Also, I don't suppose there is any way to delay by any finer resolution than millis? (Which means we essentially get just ~3 bits of entropy here; a random integer between 0 and 8.) Chris, is that fine enough?
Patchset #10 (id:250001) has been deleted
On 2017/04/12 05:59:52, Matt Giuca wrote: > On 2017/04/12 05:48:14, Ted C wrote: > > On 2017/04/12 05:37:54, Matt Giuca wrote: > > > On 2017/04/12 01:54:45, Ted C wrote: > > > > I guess I was thinking that you would guarantee a specific time and if it > > > takes > > > > longer than you just return the default no answer. > > > > > > > > <snip> > > > > > > > > I was thinking that you would just guarantee a time. The problem would be > > > that > > > > if devices are always slower than that, it would take a cycle or more > > complex > > > > code to bump up the threshold. Again, I am no security expert and likely > > > saying > > > > things that are just as flawed for other reasons. > > > > > > I think that would be fine from a security standpoint but it would make the > > > results of the call flaky; if you have a slow device or something going on > in > > > the background, it may simply never work, depending on the timer we set. I'd > > > prefer to keep the current approach and make the result deterministic. > > > > > > Thanks for the suggestion about AsyncTask. This API very neatly encapsulated > > the > > > problem of moving that off to a separate thread. I've done that in a > separate > > CL > > > (since this is a pre-existing issue unrelated to the timing delay): > > > https://codereview.chromium.org/2813153002/ > > > > > > Instead of using Handler.postDelayed, I figured I could just call > > Thread.sleep() > > > from inside the background thread. I tested it (with a 3 second sleep) and > it > > > does not block the UI thread, nor show a red flash. So I am happy with this > > > approach now. PTAL. > > > > > > (Note: If possible I would like to be able to land this before branch point > > > tomorrow :) Otherwise this will have to be merged or the feature disabled > for > > a > > > milestone.) Thanks in advance! > > > > The gotcha with AsyncTask is that it either is using a shared thread pool or > is > > serial. By doing thread.sleep we are unnecessarily clogging up resources. I > > think to be the best citizen we should post the delay instead of sleeping. > > OK that's fair, but now that I am on a background thread, can I just do a simple > delay on the same thread without having to figure out how to create a Handler on > a different thread: > > new Handler().postDelayed(<call the callback>, delayMillis); I couldn't do this on the background thread (because the body of AsyncTask needs to be synchronous). So I modified the code to record the total amount of delay time in an AtomicInteger, then call Handler.postDelayed on the UI thread at the end. This should not block the UI thread for the computation, and not block any thread for the sleep. Is this OK now? > > ? > > Also, I don't suppose there is any way to delay by any finer resolution than > millis? (Which means we essentially get just ~3 bits of entropy here; a random > integer between 0 and 8.) Chris, is that fine enough? Aha, by "fine" I mean "fine grained", not "that's a fine algorithm you got there". Thanks.
https://codereview.chromium.org/2802603002/diff/270001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:85: new AsyncTask<Void, Void, RelatedApplication[]>() { I "think" it would be slightly cleaner to change the last template var to Pair<ReleatedApplication[], Integer>. Then you can pass the values in the same way. FWIW, you could also just keep the variable as a member variable within the async task to avoid having to have it final and outside of the object. https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:322: return new Handler().postDelayed(r, delayMillis); Use can use ThreadUtils.postOnUiThreadDelayed here to avoid creating a new handler.
LGTM https://codereview.chromium.org/2802603002/diff/270001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:82: if (sSalt == null) { I don't remember my Java: can you declare |sSalt| inside this function, static, as you could in C, to ensure that it is visible only inside this function? If not, no big deal. https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:85: } catch (IOException e) { Nit: Could coalesce these 2 identical catch blocks? } catch (Exception e) { // comment... thrrow new ... }
Patchset #11 (id:290001) has been deleted
https://codereview.chromium.org/2802603002/diff/270001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:85: new AsyncTask<Void, Void, RelatedApplication[]>() { On 2017/04/12 15:53:50, Ted C wrote: > I "think" it would be slightly cleaner to change the last template var to > Pair<ReleatedApplication[], Integer>. Then you can pass the values in the same > way. > > FWIW, you could also just keep the variable as a member variable within the > async task to avoid having to have it final and outside of the object. Done. Thanks, a good suggestion. https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:322: return new Handler().postDelayed(r, delayMillis); On 2017/04/12 15:53:50, Ted C wrote: > Use can use ThreadUtils.postOnUiThreadDelayed here to avoid creating a new > handler. Done. https://codereview.chromium.org/2802603002/diff/270001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java (right): https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:82: if (sSalt == null) { On 2017/04/12 23:39:08, palmer wrote: > I don't remember my Java: can you declare |sSalt| inside this function, static, > as you could in C, to ensure that it is visible only inside this function? If > not, no big deal. You can, but I need to access it from setGlobalSaltForTesting() as well. So, not doing this. https://codereview.chromium.org/2802603002/diff/270001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/PackageHash.java:85: } catch (IOException e) { On 2017/04/12 23:39:08, palmer wrote: > Nit: Could coalesce these 2 identical catch blocks? > > } catch (Exception e) { > // comment... > thrrow new ... > } Don't want to over-generalize. However, I just learned a new syntax: catch (Type1 | Type2 e) #til
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/13 06:11:03, Ted C wrote: > lgtm Thanks for this when it's probably late at night for you.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/2802603002/#ps330001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 330001, "attempt_start_ts": 1492064855901460,
"parent_rev": "c73d5721559290e45c7d3aa8b5d1a5ef6caaade8", "commit_rev":
"245e999d5e19bd5f4bd44feed0336b2156581df1"}
Message was sent while issue was closed.
Description was changed from ========== getInstalledRelatedApps: Introduce random delay to stop timing attacks. Queries to this API have a sleep of 0--8ms, randomly chosen based on a SHA-256 hash of the app ID, salted with a unique stable device ID. This prevents websites from being able to determine whether a non-related app is installed based on the time the query takes. The website is unable to compare the timing of different apps because they have different random delays, and unable to predict the random delay for a given device, because the salt is unknown. BUG=707071 ========== to ========== getInstalledRelatedApps: Introduce random delay to stop timing attacks. Queries to this API have a sleep of 0--8ms, randomly chosen based on a SHA-256 hash of the app ID, salted with a unique stable device ID. This prevents websites from being able to determine whether a non-related app is installed based on the time the query takes. The website is unable to compare the timing of different apps because they have different random delays, and unable to predict the random delay for a given device, because the salt is unknown. BUG=707071 Review-Url: https://codereview.chromium.org/2802603002 Cr-Commit-Position: refs/heads/master@{#464322} Committed: https://chromium.googlesource.com/chromium/src/+/245e999d5e19bd5f4bd44feed033... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:330001) as https://chromium.googlesource.com/chromium/src/+/245e999d5e19bd5f4bd44feed033...
Message was sent while issue was closed.
On 2017/04/13 06:34:19, commit-bot: I haz the power wrote: > Committed patchset #12 (id:330001) as > https://chromium.googlesource.com/chromium/src/+/245e999d5e19bd5f4bd44feed033... Neat idea! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
