Closed Bug 1270836 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference after null check] In function ssl_ConfigCertByUsage

Categories

(NSS :: Libraries, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Tracking Status
firefox49 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1358966)

Attachments

(2 files, 2 obsolete files)

The Static Analysis tool Coverity added that pointer |data| is dereferenced after is null checked:

>>    if (data) {
>>        /* Take a (shallow) copy so that we can play with it */
>>        memcpy(&arg, data, sizeof(arg));
>>    }

and the dereference takes place:

>>    /* |data->authType| has to either agree or be ssl_auth_null. */
>>    if (data->authType != ssl_auth_null && data->authType != arg.authType) {
>>        PORT_SetError(SEC_ERROR_INVALID_ARGS);
>>        return SECFailure;
>>    }

The version that we have in our tree for NSS is very different from the latest version of NSS, and in the latest version this issue is no longer present. So until we push a new update of NSS to our tree maybe we should add this fix.
Attachment #8749645 - Flags: review?(emaldona)
Component: Security → Libraries
Product: Core → NSS
Version: Trunk → trunk
(In reply to Andi-Bogdan Postelnicu from comment #0)
> The Static Analysis tool Coverity added that pointer |data| is dereferenced
> after is null checked:
> 
> >>    if (data) {
> >>        /* Take a (shallow) copy so that we can play with it */
> >>        memcpy(&arg, data, sizeof(arg));
> >>    }
> 
> and the dereference takes place:
> 
> >>    /* |data->authType| has to either agree or be ssl_auth_null. */
> >>    if (data->authType != ssl_auth_null && data->authType != arg.authType) {
> >>        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> >>        return SECFailure;
> >>    }
> 
> The version that we have in our tree for NSS is very different from the
> latest version of NSS, and in the latest version this issue is no longer
> present. So until we push a new update of NSS to our tree maybe we should
> add this fix.

The patch looks good at first sight but before I give an r+ I have a few questions:
What is the version of NSS that you have in the tree? 
How come in latest version problem isn't present, given that te code in question is essentially the same? 
Are we using different version of Coverity?
Flags: needinfo?(bogdan.postelnicu)
My bad! When i first look i didn't find the issues mentioned in the scan for nss coverity and i didn't look in the actual file from NSStrunk. We are using version NSS_3_24_RC0.
I should better submit this fix in the original trunk file rather than our own.
Flags: needinfo?(bogdan.postelnicu)
Attachment #8750211 - Flags: review?(emaldona)
Attachment #8749645 - Flags: review?(emaldona) → review+
Comment on attachment 8749645 [details]
MozReview Request: Bug 1270836 - prevent null pointer dereference on |data|. r?emaldona

https://reviewboard.mozilla.org/r/51081/#review48105

NIT: fix trailing whitespace
Attachment #8750211 - Attachment is obsolete: true
Attachment #8750211 - Flags: review?(emaldona)
Comment on attachment 8749645 [details]
MozReview Request: Bug 1270836 - prevent null pointer dereference on |data|. r?emaldona

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51081/diff/1-2/
Attachment #8749645 - Attachment description: MozReview Request: Bug 1270836 - prevent null pointer dereference on |data|. r?emaldona@redhat.com → MozReview Request: Bug 1270836 - prevent null pointer dereference on |data|. r?emaldona
If this is a change to trunk NSS, it should have been checked in there, not into mozilla-central/inbound.
Flags: needinfo?(bogdan.postelnicu)
Attachment #8750211 [details] [diff] is for NSS repo, but i don't know why the review was cancelled.
Flags: needinfo?(bogdan.postelnicu)
(In reply to Andi-Bogdan Postelnicu from comment #10)
> Attachment #8750211 [details] [diff] [diff] is for NSS repo, but i don't know why
> the review was cancelled.

There were two patches and I had approved one of them so I obsoleted the other to make Bugzilla stop reminding me I had do a review I had already done. I might had chosen the wrong one.
Comment on attachment 8750211 [details] [diff] [review]
prevent null pointer dereference on |data|

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

Don't worry, i don't see any reason why the patch that has been submitted to mozilla inbound should be backed out.
Attachment #8750211 - Flags: review?(emaldona)
Attachment #8751306 - Flags: review?(emaldona)
Comment on attachment 8750211 [details] [diff] [review]
prevent null pointer dereference on |data|

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

Don't worry, i don't see any reason why the patch that has been submitted to mozilla inbound should be backed out.
Attachment #8750211 - Flags: review?(emaldona)
Comment on attachment 8751306 [details] [diff] [review]
prevent null pointer dereference on |data|

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

::: lib/ssl/sslcert.c
@@ +451,5 @@
>          PORT_SetError(SEC_ERROR_INVALID_ARGS);
>          return SECFailure;
>      }
>      /* |data->authType| has to either agree or be ssl_auth_null. */
> +    if (data && data->authType != ssl_auth_null && 

nitpick: trailing whitespace
Attachment #8751306 - Flags: review?(emaldona) → review+
Attachment #8751306 - Attachment is obsolete: true
Attachment #8751309 - Flags: review?(emaldona) → review+
Keywords: checkin-needed
(In reply to Andi-Bogdan Postelnicu from comment #14)
> 
> Don't worry, i don't see any reason why the patch that has been submitted to
> mozilla inbound should be backed out.

The reason is, that NSS in mozilla-central must never be a fork of NSS, and by commiting directly into m-c, without to NSS, you're essentially creating a fork.

Also, NSS in m-c is updated in whole to snapshots of the NSS repository.
Usually, nobody will notice individual commits to nss in m-c, they will be simply overwritten whenever a new snapshot is landed.

We have an exception rule. Very urgent build or code fixes may be landed temporarily into m-c, ahead of landing to a new snapshot of NSS, but only AFTER it has been committed, to the NSS repository, too.

All such early landings must be recorded in file security/nss/README in the mozilla tree.

We never allow new API additions by that mechanism.

I only noticed your landing, because I'm fixing m-c to use the final RTM tag of NSS 3.24, and I saw your change, which was unexpected.

I'll land your change into NSS trunk which will target NSS 3.25.
Your change will appear again in m-c as soon as we land a NSS 3.25 beta version of NSS into m-c.
(In reply to Kai Engert (:kaie) from comment #18)
> (In reply to Andi-Bogdan Postelnicu from comment #14)
> > 
> > Don't worry, i don't see any reason why the patch that has been submitted to
> > mozilla inbound should be backed out.
> 
> The reason is, that NSS in mozilla-central must never be a fork of NSS, and
> by commiting directly into m-c, without to NSS, you're essentially creating
> a fork.
> 
> Also, NSS in m-c is updated in whole to snapshots of the NSS repository.
> Usually, nobody will notice individual commits to nss in m-c, they will be
> simply overwritten whenever a new snapshot is landed.
> 
> We have an exception rule. Very urgent build or code fixes may be landed
> temporarily into m-c, ahead of landing to a new snapshot of NSS, but only
> AFTER it has been committed, to the NSS repository, too.
> 
> All such early landings must be recorded in file security/nss/README in the
> mozilla tree.
> 
> We never allow new API additions by that mechanism.
> 
> I only noticed your landing, because I'm fixing m-c to use the final RTM tag
> of NSS 3.24, and I saw your change, which was unexpected.
> 
> I'll land your change into NSS trunk which will target NSS 3.25.
> Your change will appear again in m-c as soon as we land a NSS 3.25 beta
> version of NSS into m-c.

thanks you!
The change from this bug was backed out from m-i as part of finalizing bug 1258375.

(In reply to Kai Engert (:kaie) from comment #20)
> https://hg.mozilla.org/projects/nss/rev/0768f4a49f96

This landed it into NSS trunk for NSS 3.25
It will land again into m-i when we uplift a newer NSS snapshot to m-i.
You need to log in before you can comment on or make changes to this bug.