Potential read of uninitialized data in cert viewer

RESOLVED FIXED

Status

()

Core
Security: PSM
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

({crash, fixed1.8.1.12})

1.8 Branch
crash, fixed1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Firefox 2 has the some code, setting to 1.8 branch.
Version: Trunk → 1.8 Branch

Comment 2

10 years ago
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));
(Assignee)

Comment 3

10 years ago
Created attachment 293033 [details] [diff] [review]
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.
Attachment #293033 - Flags: review?(wtc)
(Assignee)

Comment 4

10 years ago
Created attachment 293035 [details] [diff] [review]
Patch v2

I should compile my patches before I attach them...
Changed SECStatus to PRStatus.
Attachment #293033 - Attachment is obsolete: true
Attachment #293035 - Flags: review?(wtc)
Attachment #293033 - Flags: review?(wtc)

Comment 5

10 years ago
Comment on attachment 293035 [details] [diff] [review]
Patch v2

r=wtc.

The local variable name "IPStatus" should be spelled in lowercase.
Perhaps just "status"?
Attachment #293035 - Flags: review?(wtc) → review+
(Assignee)

Comment 6

10 years ago
Requesting blocker status for this crasher bug, patch in hand.
Flags: blocking1.9?
(Assignee)

Comment 7

10 years ago
Created attachment 293144 [details] [diff] [review]
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+.
Attachment #293144 - Flags: review+
Attachment #293144 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

a=beltzner
Attachment #293144 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

10 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Whiteboard: [sg:??]
(Assignee)

Comment 11

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

Dan, thanks for catching. Requesting approval.
Attachment #293144 - Flags: approval1.8.1.12?
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Whiteboard: [sg:??] → [sg:moderate?]
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 on attachment 293144 [details] [diff] [review]
Patch v2 + variable renamed

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #293144 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 14

10 years ago
checked in to 1.8 branch
Keywords: fixed1.8.1.12
QA isn't sure how to verify this fix. Dveditz, did you get any more information?
Group: security

Comment 16

10 years ago
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.
You need to log in before you can comment on or make changes to this bug.