1110212 - (CVE-2015-0800) Fennec name resolver uses weak DNS randomness (see CVE-2012-2808)

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Daniel Stenberg [:bagder]]

Our own DNS resolving code is based on an older Android/bionic version.

That older code uses a weak random algorithm for the ID in the DNS resolves, which makes it vulnerable to DNS poisoning attacks. See URL that links to an IBM report about this problem.

This problem has since been fixed in the upstream bionic tree.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: sec-strong-random-dns-resolver

This is the upstream random generator code with increased entropy.

Comment 2

[Daniel Stenberg [:bagder]]

I realize this change comes from bionic, but I wonder what the reason is for reading a full (32 bit) int from /dev/urandom and then use the lower 16 bits from that only.

Comment 3

[Eugen Sawin [:esawin]]

(In reply to Daniel Stenberg [:bagder] from comment #2)
> I realize this change comes from bionic, but I wonder what the reason is for
> reading a full (32 bit) int from /dev/urandom and then use the lower 16 bits
> from that only.

res_randomid is called in res_init and some other performance-critical code, so /dev/urandom looks like the right choice (non-blocking), and the random id is used to set rest_state::id, which is a u_short.

Comment 4

[Eugen Sawin [:esawin]]

I meant res_state, as defined in resolv_private.h.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8535179 [details] [diff] [review]
sec-strong-random-dns-resolver

Review of attachment 8535179 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look fine to me. For the record, I read the paper that describes the vulnerability. - http://blog.watchfire.com/files/androiddnsweakprng.pdf.

Eugen, can you find the revision in AOSP that this was added.

Also, how often is the code expected to fall through to the old random generator? I'm wondering if we should assert if the answer is never.

Daniel, please chime in if you have any other comments.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #5)

> Eugen, can you find the revision in AOSP that this was added.

I forgot to say why :) Can you add a one liner in the code to reference that revision, please.

Comment 7

[Daniel Stenberg [:bagder]]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #5)

> Also, how often is the code expected to fall through to the old random
> generator? I'm wondering if we should assert if the answer is never.

To me it seems like it should be never.

> Daniel, please chime in if you have any other comments.

I'm agreeing with you, it all looks fine.

Comment 8

[Eugen Sawin [:esawin]]

> Eugen, can you find the revision in AOSP that this was added.
Will do.

> Also, how often is the code expected to fall through to the old random
> generator? I'm wondering if we should assert if the answer is never.
Currently, we set ANDROID_CHANGES, so we will never execute the old path. I can add an assertion to verify that.

Comment 9

[Daniel Stenberg [:bagder]]

(In reply to Eugen Sawin [:esawin] from comment #8)

> Currently, we set ANDROID_CHANGES, so we will never execute the old path.

... only if "dev/urandom" can't be opened or if the read() from it fails or gets aborted by EINTR more than 5 times...

Comment 10

[Eugen Sawin [:esawin]]

(In reply to Daniel Stenberg [:bagder] from comment #9)
> (In reply to Eugen Sawin [:esawin] from comment #8)
> 
> > Currently, we set ANDROID_CHANGES, so we will never execute the old path.
> 
> ... only if "dev/urandom" can't be opened or if the read() from it fails or
> gets aborted by EINTR more than 5 times...

You're right, so asserting that this conditions are never met should be reasonable as that would hint at some underlying problem.

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: This is something that we should evaluate for the Firefox 34.0.1 respin

Comment 12

[Patrick McManus [:mcmanus]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> [Tracking Requested - why for this release]: This is something that we
> should evaluate for the Firefox 34.0.1 respin

I don't think this should be rushed into release channel.. this is a long standing problem, with no evidence of exploit and a change being made a pretty sensitive part of the system. and even when 'fixed' dns is still susceptible to many other spoofing attacks so its reliability isn't anything we really build a trust guarantee around.

Comment 13

[Eugen Sawin [:esawin]]

Upon further investigation, I found out that after the update in 2012 (which this patch is based on), Bionic is now shipping with the OpenBSD-based random generator for Android L.

I think updating to the new implementation would be feasible for us technically, but I haven't looked further into the reasoning for this change yet.

Comment 14

[Daniel Veditz [:dveditz]]

guessing at a severity, sec-low? sec-moderate?

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Patrick McManus [:mcmanus] from comment #12)
> (In reply to Kevin Brosnan [:kbrosnan] from comment #11)
> > [Tracking Requested - why for this release]: This is something that we
> > should evaluate for the Firefox 34.0.1 respin
> 
> I don't think this should be rushed into release channel.. this is a long
> standing problem, with no evidence of exploit and a change being made a
> pretty sensitive part of the system. and even when 'fixed' dns is still
> susceptible to many other spoofing attacks so its reliability isn't anything
> we really build a trust guarantee around.

I agree with Patrick. I have marked this tracking- and wontfix for 34.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

sec-low, not tracking.

Comment 17

[Eugen Sawin [:esawin]]

Attachment: sec-strong-random-dns-resolver

Added assertion and AOSP revision/changeset reference.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb6ad877e26f

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8538508 [details] [diff] [review]
sec-strong-random-dns-resolver

Review of attachment 8538508 [details] [diff] [review]:
-----------------------------------------------------------------

::: other-licenses/android/res_init.c
@@ +771,5 @@
> +	if (status != -1) {
> +		return output;
> +	}
> +  /* There was an unexpected issue with reading from /dev/urandom. */
> +  assert(status != -1);

nit: Indentation is off here.

Comment 19

[Eugen Sawin [:esawin]]

Attachment: sec-strong-random-dns-resolver

Fixed indentation.

Comment 20

[Eugen Sawin [:esawin]]

https://hg.mozilla.org/integration/fx-team/rev/ee4fe2ec168e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee4fe2ec168e
https://hg.mozilla.org/mozilla-central/rev/b58098a4ca40

Comment 22

[Al Billings [:abillings - ex-MoCo]]

The CVE in the title doesn't seem to exist. Is it a typo? You added it, Dan.

Comment 23

[Daniel Veditz [:dveditz]]

I have no idea why cve.mitre.org and web.nvd.nist.gov don't know about it, but it was all over the news under that CVE number at the time. Here's the original source

http://blog.watchfire.com/wfblog/2012/07/android-dns-poisoning-randomness-gone-bad-cve-2012-2808.html 
http://seclists.org/fulldisclosure/2012/Jul/334