406290 - Potential read of uninitialized data in cert viewer

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Kai Engert (:KaiE:)]

File mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp

ProcessGeneralName calls into PR_NetAddrToString (twice).

The return value from PR_NetAddrToString it not being checked.

Even if PR_NetAddrToString fails, the code will the contents of "buf" to a string.

"buf" is uninitialized and might not contain zero.

Comment 1

[Kai Engert (:KaiE:)]

Firefox 2 has the some code, setting to 1.8 branch.

Comment 2

[Wan-Teh Chang]

PR_NetAddrToString is unlikely to fail there, but we should
check its return value.  What should we do if PR_NetAddrToString
fails?  Store an empty string in "buf", or just call
ProcessRawBytes?  The latter seems better.  Since we're in a
switch statement, we can do that using "break" if PR_NetAddrToString
succeeds:

  if (current->name.other.len == 4) {
    addr.inet.family = PR_AF_INET;
    memcpy(&addr.inet.ip, current->name.other.data, current->name.other.len);
    if (PR_NetAddrToString(&addr, buf, sizeof(buf)) == PR_SUCCESS) {
      value.AssignASCII(buf);
      break;
    }
  } else if (current->name.other.len == 16) {
    addr.ipv6.family = PR_AF_INET6;
    addr.ipv6.flowinfo = 0;
    memcpy(&addr.ipv6.ip, current->name.other.data, current->name.other.len);
    addr.ipv6.scope_id = 0;
    if (PR_NetAddrToString(&addr, buf, sizeof(buf)) == PR_SUCCESS) {
      value.AssignASCII(buf);
      break;
    }
  }
  /* invalid IP address */
  ProcessRawBytes(nssComponent, &current->name.other, value);
  break;

Note that in the IPv6 case I added two lines to set "flowinfo"
and "scope_id" to 0.  Alternatively you can just zero the
whole "addr" at the beginning:

  memset(&addr, 0, sizeof(addr));

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Wan-Teh, thanks a lot for your helpful comments!

> What should we do if PR_NetAddrToString fails?  Store an empty string in 
> "buf", or just call ProcessRawBytes?  
> The latter seems better.  

I agree.


> Note that in the IPv6 case I added two lines to set "flowinfo"
> and "scope_id" to 0.  Alternatively you can just zero the
> whole "addr" at the beginning:
> 
>   memset(&addr, 0, sizeof(addr));

If there are mutliple members that need to be initialized in order to make the conversion work, I rather prefer the memset.


> Since we're in a switch statement, we can do that using "break"

I tend to avoid jumps, in the patch I use a status variable to avoid it.

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v2

I should compile my patches before I attach them...
Changed SECStatus to PRStatus.

Comment 5

[Wan-Teh Chang]

Comment on attachment 293035 [details] [diff] [review]
Patch v2

r=wtc.

The local variable name "IPStatus" should be spelled in lowercase.
Perhaps just "status"?

Comment 6

[Kai Engert (:KaiE:)]

Requesting blocker status for this crasher bug, patch in hand.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v2 + variable renamed

Wan-Teh, thanks for your review. I renamed the variable as you suggested.
No other changes in this patch, carrying forward r+.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

a=beltzner

Comment 9

[Kai Engert (:KaiE:)]

fixed

Comment 10

[Daniel Veditz [:dveditz]]

We want this on the 1.8 branch you said, right? Please request approval on this patch if it applies, otherwise on a new branch patch.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

Dan, thanks for catching. Requesting approval.

Comment 12

[Daniel Veditz [:dveditz]]

Is there a testcase for this? I guess we can run in a debug build with uninitialized variable checking, or maybe something like gflags.exe on windows will check that.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 14

[Kai Engert (:KaiE:)]

checked in to 1.8 branch

Comment 15

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

QA isn't sure how to verify this fix. Dveditz, did you get any more information?

Comment 16

[Wan-Teh Chang]

Comment on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

Al: just verify that this patch has been checked in on the
MOZILLA_1_8_BRANCH.  It is not easy to verify that we're not
using the buffer 'buf' uninitialized by testing.