CERT_ImportCerts return value not checked in ImportCertsIntoPermanentStorage

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: keeler, Assigned: mz_mhs-ctb)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

497 static 
498 SECStatus 
499 ImportCertsIntoPermanentStorage(const ScopedCERTCertList &certChain, const SECCertUsage usage,
500                                const PRBool caOnly)
501 {
502   CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
...
523   CERT_ImportCerts(certdb, usage, chainLen,
524                    rawArray,  nullptr, true, caOnly, nullptr);
525 
526   PORT_Free(rawArray);   
527   return SECSuccess;
528 }

CERT_ImportCerts can fail, and its return value is checked elsewhere. It would be best to do it here as well.
(While we're at it, it doesn't look like all calls to ImportCertsIntoPermanentStorage are checked, either.)
Posted patch Patch (obsolete) — Splinter Review
Attachment #826390 - Flags: review?(brian)
Attachment #826390 - Flags: review?(brian) → review?(dkeeler)
Comment on attachment 826390 [details] [diff] [review]
Patch

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

Thanks for the patch. I'd like a couple of changes, so r- for now.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +525,5 @@
>  
> +  PORT_Free(rawArray);
> +  if (srv != SECSuccess) {
> +    return SECFailure;
> +  }

You can just return srv here.

@@ +804,5 @@
>      }
>  
> +    rv = ImportCertsIntoPermanentStorage(certChain, certUsageAnyCA, true);
> +    if (rv != SECSuccess) {
> +      continue;

Actually, instead of continuing, I think we should treat this like nsNSSCertificateDB::ImportEmailCertificate and break out of the loop if this fails (and the eventual return value should be dependent on if we ever fail here).
Attachment #826390 - Flags: review?(dkeeler) → review-
Posted patch Patch (v2) (obsolete) — Splinter Review
Like this?
Attachment #826390 - Attachment is obsolete: true
Attachment #827108 - Flags: review?(dkeeler)
Comment on attachment 827108 [details] [diff] [review]
Patch (v2)

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

r=me with the nit fixed. Thanks for working on this!

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +520,5 @@
>         chainNode = CERT_LIST_NEXT(chainNode), i++) {
>      rawArray[i] = &chainNode->cert->derCert;
>    }
> +  SECStatus srv = CERT_ImportCerts(certdb, usage, chainLen,
> +                                   rawArray,  nullptr, true, caOnly, nullptr);

Nit: it looks like you can move at least rawArray onto the line above where it is (but don't go beyond 80 characters), which will make that second line shorter. Also, there's two spaces before the first nullptr.
Attachment #827108 - Flags: review?(dkeeler) → review+
Sorry about using checkin? again, but I can't set checkin-needed.
Attachment #828053 - Flags: checkin?
Attachment #827108 - Attachment is obsolete: true
Comment on attachment 828053 [details] [diff] [review]
Patch with nit fixed. r=dkeeler

Please just use the checkin-needed bug keyword for single-patch bugs like this one. Also, I fixed the bug number in the commit message for you :)
Attachment #828053 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/7acc8aef5e5d
Assignee: nobody → mz_mhs-ctb
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7acc8aef5e5d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.