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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1270836 - prevent null pointer dereference on |data|. r?emaldona

Review commit: https://reviewboard.mozilla.org/r/51081/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51081/

Comment 2

[Elio Maldonado]

(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?

Comment 3

[Andi [:andi]]

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.

Comment 4

[Andi [:andi]]

Attachment: prevent null pointer dereference on |data|

Comment 5

[Elio Maldonado]

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

Comment 6

[Andi [:andi]]

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/

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75452b28001f

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75452b28001f

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

If this is a change to trunk NSS, it should have been checked in there, not into mozilla-central/inbound.

Comment 10

[Andi [:andi]]

Attachment #8750211 [details] [diff] is for NSS repo, but i don't know why the review was cancelled.

Comment 11

[Elio Maldonado]

(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 12

[Andi [:andi]]

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.

Comment 13

[Andi [:andi]]

Attachment: prevent null pointer dereference on |data|

Comment 14

[Andi [:andi]]

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.

Comment 15

[Elio Maldonado]

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

Comment 16

[Andi [:andi]]

Attachment: prevent null pointer dereference on |data|

Comment 17

[Andi [:andi]]

https://bugzilla.mozilla.org/attachment.cgi?id=8751309&action=edit
Should be checkedin the nss repo!

Comment 18

[Kai Engert (:KaiE:)]

(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.

Comment 19

[Andi [:andi]]

(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!

Comment 20

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/0768f4a49f96

Comment 21

[Kai Engert (:KaiE:)]

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.