Closed Bug 1012549 Opened 11 years ago Closed 10 years ago

[Wifi] Support importing PKCS#12 file format.

Categories

(Firefox OS Graveyard :: Wifi, defect)

Product:
Firefox OS Graveyard
Component:
Wifi
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(tracking-b2g:backlog, firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S7 (6mar)
Project Flags:
tracking-b2g backlog
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: chucklee, Assigned: chucklee)

References

Details

(Whiteboard: [p=13])

Attachments

(4 files, 49 obsolete files)

0002. Support list PKCS12 certificate. r=vchang
1014 bytes, patch
Details | Diff | Splinter Review
0003. Support delete PKCS12 certificate. V3. r=vchang r=dkeeler
2.00 KB, patch
Details | Diff | Splinter Review
0001. Support import PKCS12 certificate. V8. r=dkeeler. r=vchang
11.43 KB, patch
Details | Diff | Splinter Review
0004. Support read private key in keystore. V10. r=dkeeler. r=qdot
24.86 KB, patch
Details | Diff | Splinter Review
Attachment #8427660 - Attachment description: 0001. Allow read private key through PKCS12 decoder iterator. → WIP - 0001. Allow read private key through PKCS12 decoder iterator.
Hi David, I am trying to implement import PKCS#12, both certificate and private key, with customized nickname for wifi usage. Current WIP is able to import/read/delete certificates correctly, and it seems the private key import is succeeded. But I can't read private key with assigned nickname by |CERT_FindCertByNickname()|, also it is not shown in the certificate lists. Can you provide some feedback if I am doing this right, or I should do this in another way? Thanks.
Comment on attachment 8427660 [details] [diff] [review] WIP - 0001. Allow read private key through PKCS12 decoder iterator. Review of attachment 8427660 [details] [diff] [review]: ----------------------------------------------------------------- Hi Chuck, A couple of things: 1. I'm all for removing unnecessary whitespace like you've done in this patch, but it actually makes it hard to review. I would suggest first doing a whitespace-changes-only patch and then making your changes in a separate patch. 2. I'm not very familiar with this part of NSS, so I'm probably not the best person to give feedback. I would ask :cviecco or someone listed here: https://wiki.mozilla.org/Modules/All#security (Wan-Teh and Ryan are particularly responsive in my experience).
Attachment #8427660 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 8427662 [details] [diff] [review] WIP - 0002. Support import PKCS12 certificate. Review of attachment 8427662 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +97,5 @@ > + // Import certificate. > + return ImportCert(cert); > + } > + > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem &aItem) Is there a better way to do this (i.e. maybe a library function)? Why are we treating binary data as a wide-character string anyway? I see - this is converting a password to a SECItem. It's probably best to use a wide-to-narrow string converter from one of our string libraries and then copy that data to the SECItem. @@ +112,5 @@ > + aItem.data[2*i ] = (unsigned char)(stringData[i] << 8); > + aItem.data[2*i+1] = (unsigned char)(stringData[i]); > + } > +#endif > + // Make it null-terminated SECItems don't need to be null-terminated - they're just data. @@ +120,5 @@ > + > + nsresult ImportPkcs12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + // Get password. > + ScopedAutoSECItem passwordItem; ScopedSECItem is probably better to use. I'm not even sure why we have ScopedAutoSECItem. @@ +129,5 @@ > + &passwordItem, nullptr, nullptr, > + NULL, NULL, NULL, NULL, NULL)); > + > + if (!p12dcx) { > + return NS_ERROR_UNEXPECTED; NS_ERROR_FAILURE might be better @@ +294,5 @@ > + > + ScopedAutoSECItem nicknameItem; > + SECITEM_AllocItem(nicknameItem, nickname.Length() + 1); > + memcpy(nicknameItem.data, nickname.get(), nickname.Length()); > + nicknameItem.data[nickname.Length()] = 0; Same as before: 1. Use ScopedSECItem 2. SECItem is data, so it shouldn't be null-terminated Also, you should be able to do this: ScopedSECItem nicknameItem(SECITEM_AllocItem(nullptr, nullptr, nickname.Length()); @@ +304,5 @@ > + if (srv != SECSuccess) { > + return MapSECStatus(srv); > + } > + } else { > + srv = PK11_ChangePW(aPrivateKey->slot, "", ""); Why is this necessary? @@ +313,5 @@ > + > + SECKEYPrivateKey *privateKey; > + srv = PK11_ImportEncryptedPrivateKeyInfoAndReturnKey(aPrivateKey->slot, > + aPrivateKey->safeBagContent.pkcs8ShroudedKeyBag, > + aPrivateKey->pwitem, &nicknameItem, pubValue, PR_TRUE, This should be nicknameItem.get() if just nicknameItem doesn't work directly. @@ +314,5 @@ > + SECKEYPrivateKey *privateKey; > + srv = PK11_ImportEncryptedPrivateKeyInfoAndReturnKey(aPrivateKey->slot, > + aPrivateKey->safeBagContent.pkcs8ShroudedKeyBag, > + aPrivateKey->pwitem, &nicknameItem, pubValue, PR_TRUE, > + PR_FALSE, pubKey->keyType, cert->keyUsage, &privateKey, wincx); nit: watch out for long lines like this. Since the function name is so long, you can just have all of the arguments be indented two spaces from where "srv" is. @@ +323,5 @@ > + aPrivateKey->installed = PR_TRUE; > + return NS_OK; > + } > + > + nit: unnecessary blank line
Attachment #8427662 - Flags: feedback?(dkeeler) → feedback+
(In reply to Chuck Lee [:chucklee] from comment #3) > But I can't read private key with assigned nickname by > |CERT_FindCertByNickname()|, also it is not shown in the certificate lists. Are you using certutil -K for this? If the certificate isn't kept permanently, NSS might consider the key orphaned. Other than that, I don't know what would be wrong off the top of my head. Again, there are others who are more familiar with NSS (see the list of peers I linked to earlier).
blocking-b2g: --- → backlog
Comment on attachment 8427662 [details] [diff] [review] WIP - 0002. Support import PKCS12 certificate. Review of attachment 8427662 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +112,5 @@ > + aItem.data[2*i ] = (unsigned char)(stringData[i] << 8); > + aItem.data[2*i+1] = (unsigned char)(stringData[i]); > + } > +#endif > + // Make it null-terminated The import process will fail at |SEC_PKCS12DecoderUpdate()| if remove the null-terminated part, using |CopyUTF16toUTF8()| like nickname below also fails. Also I can't find a function in the string library providing this functionality. This seems also confuses PKCS12Blob[1]. So I have to keep this function for now. [1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsPKCS12Blob.cpp#415
Clear nits to patch 0001.
Attachment #8427660 - Attachment is obsolete: true
Attachment #8429156 - Flags: feedback?(wtc)
Attachment #8429156 - Flags: feedback?(ryan.sleevi)
Attachment #8427662 - Attachment is obsolete: true
Attachment #8429157 - Flags: feedback?(wtc)
Attachment #8429157 - Flags: feedback?(ryan.sleevi)
Hi Wan-Teh, Ryan, I am trying to implement import PKCS#12, both certificate and private key, with customized nickname for wifi usage, the problem now is I can't read private key with assigned nickname by |CERT_FindCertByNickname()|, also it is not shown in the certificate lists returned from |PK11_ListCerts|. (I am trying but can't run certutil on dev phone now) Can you provide some feedback if I am doing this right, or I should do this in another way? Thanks.
After some trying, now I can user certutil on Firefox OS. > $ adb push objdir-gecko/security/nss/cmd/certutil/certutil /sdcard/certutil > $ adb shell chmod 777 /sdcard/certutil > $ adb shell mount -o exec,remount /mnt/sdcard/ And I can get certificate list and private key list, so it seems the import do success. I'll check how certutil works. > $ adb shell LD_LIBRARY_PATH=/system/b2g /sdcard/certutil -L -d /data/b2g/mozilla/u5u2gihq.default > > Certificate Nickname Trust Attributes > SSL,S/MIME,JAR/XPI > > WIFI_USERCERT_chucklee ,, > WIFI_SERVERCERT_chucklee ,, > > $ adb shell LD_LIBRARY_PATH=/system/b2g /sdcard/certutil -K -d /data/b2g/mozilla/u5u2gihq.default > certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services" > < 0> rsa 6f7145ade5cc045f3a070a2d4da5f62ddb2c1229 WIFI_USERKEY_chucklee
Hi Chuck, I'm not familiar with this code. Is there something specific you want me to be looking at?
Attachment #8429156 - Flags: feedback?(ryan.sleevi)
Attachment #8429157 - Flags: feedback?(ryan.sleevi)
(In reply to Ryan Sleevi from comment #13) > Hi Chuck, > > I'm not familiar with this code. Is there something specific you want me to > be looking at? Hi Ryan, I think the most important topic is if it's ok to export safebags to pkcs12 iterator, per patch 0002(https://bugzilla.mozilla.org/attachment.cgi?id=8429156). Next thing is if you know how to export a private key out of NSS by nickname, in DER format is preferred. I am still struggling on this. Or do you know who, besides Wan-Teh, can give me opinions on these topics? Thanks.
Attached patch 0001. Fix nits in NSS. (obsolete) — — Splinter Review
Attachment #8429154 - Attachment is obsolete: true
Attachment #8456720 - Flags: review?(ryan.sleevi)
Attachment #8429156 - Attachment is obsolete: true
Attachment #8429156 - Flags: feedback?(wtc)
Attachment #8456721 - Flags: review?(ryan.sleevi)
Attachment #8455099 - Attachment is obsolete: true
Attachment #8456722 - Flags: review?(ryan.sleevi)
Attachment #8455100 - Attachment is obsolete: true
Attachment #8456723 - Flags: review?(vchang)
Attachment #8455102 - Attachment is obsolete: true
Attachment #8456725 - Flags: review?(vchang)
Attachment #8456725 - Flags: review?(ryan.sleevi)
Attachment #8455103 - Attachment is obsolete: true
Attachment #8456726 - Flags: review?(ryan.sleevi)
Attachment #8456726 - Flags: review?(kyle)
Comment on attachment 8456726 [details] [diff] [review] 0006. Support read private key in keystore. Review of attachment 8456726 [details] [diff] [review]: ----------------------------------------------------------------- This isn't something I can review. I was just on the keystore reviews for the UnixSocket work, but I'm not qualified to r+ crypto stuff.
Attachment #8456726 - Flags: review?(kyle)
Comment on attachment 8456720 [details] [diff] [review] 0001. Fix nits in NSS. Review of attachment 8456720 [details] [diff] [review]: ----------------------------------------------------------------- I'm r-'ing this, because my understanding is that we don't do pure style changes (which this touches a large number of lines, and just looks to be whitespace cleanup). f? to Bob to confirm this
Attachment #8456720 - Flags: review?(ryan.sleevi)
Attachment #8456720 - Flags: review-
Attachment #8456720 - Flags: feedback?(rrelyea)
Comment on attachment 8456721 [details] [diff] [review] 0002. Allow read private key through PKCS12 decoder iterator. Review of attachment 8456721 [details] [diff] [review]: ----------------------------------------------------------------- r- Exposing "internal" details (as the lowercase "sec_" is meant to suggest, per NSS convension) is not appropriate for the "public" APIs. You would need to define a well-defined, structured public type, that will be supported going forward.
Attachment #8456721 - Flags: review?(ryan.sleevi) → review-
Comment on attachment 8456722 [details] [diff] [review] 0003. Support import PKCS12 certificate. Review of attachment 8456722 [details] [diff] [review]: ----------------------------------------------------------------- As I've told you before, I'm not an appropriate reviewer for files outside of NSS.
Attachment #8456722 - Flags: review?(ryan.sleevi)
Comment on attachment 8456725 [details] [diff] [review] 0005. Support delete PKCS12 certificate. Review of attachment 8456725 [details] [diff] [review]: ----------------------------------------------------------------- Not a suitable reviewer.
Attachment #8456725 - Flags: review?(ryan.sleevi)
Comment on attachment 8456726 [details] [diff] [review] 0006. Support read private key in keystore. Review of attachment 8456726 [details] [diff] [review]: ----------------------------------------------------------------- Not a suitable reviewer.
Attachment #8456726 - Flags: review?(ryan.sleevi)
Use public types to provide private key info.
Attachment #8456721 - Attachment is obsolete: true
Attachment #8460054 - Flags: review?(ryan.sleevi)
Update based to data structure change in patch 0002. Hi David, Can you help to review if I use the NSS API properly? Thanks
Attachment #8456722 - Attachment is obsolete: true
Attachment #8460058 - Flags: review?(vchang)
Attachment #8460058 - Flags: review?(dkeeler)
Comment on attachment 8456725 [details] [diff] [review] 0005. Support delete PKCS12 certificate. Review of attachment 8456725 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable based on what other similar code does. ::: dom/wifi/WifiCertService.cpp @@ +418,5 @@ > + > + return NS_OK; > + } > + > + nsresult deleteCert(const nsCString &aCertNickname) nit: why have a return value if all callers just ignore it? You should probably do something more like: rv = deleteCert(...); if (NS_FAILED(rv)) { return rv; }
Attachment #8456725 - Flags: review?(dkeeler) → review+
Comment on attachment 8456726 [details] [diff] [review] 0006. Support read private key in keystore. Review of attachment 8456726 [details] [diff] [review]: ----------------------------------------------------------------- r- because this seems unnecessary - see the 6th comment in this review. ::: ipc/keystore/KeyStore.cpp @@ +134,5 @@ > return ::PERMISSION_DENIED; > } > > String8 certName(name); > + if (strncmp(certName.string(), "WIFI_USERKEY_", 13)) { The way you're doing this is confusing - if the true branch is taken, you're decoding something that isn't a user key, right? I would flip this around and use !strncmp. @@ +138,5 @@ > + if (strncmp(certName.string(), "WIFI_USERKEY_", 13)) { > + return getCertificate(certName.string(), (const uint8_t **)item, > + (int *)itemLength); > + } > + return getPrivateKey(certName.string(), (const uint8_t **)item, You shouldn't need to cast a uint8_t** to a const uint8_t** in this context @@ +139,5 @@ > + return getCertificate(certName.string(), (const uint8_t **)item, > + (int *)itemLength); > + } > + return getPrivateKey(certName.string(), (const uint8_t **)item, > + (int *)itemLength); Casting a size_t* to a int* isn't safe - check for size limitations first @@ +280,5 @@ > return SUCCESS; > } > > +static void > +RC4(uint8_t *encrypted, int dataLen, uint8_t *key, int keyLen, uint8_t *decrypted) Don't implement RC4 yourself. Don't we have a library implementation? @@ +311,5 @@ > + } > +} > + > +ResponseCode getPrivateKey(const char *aCertName, const uint8_t **aCertData, > + int *aCertDataLength) Shouldn't these be "aKeyData" and "aKeyDataLength"? @@ +344,5 @@ > + > + // Decrypt into RSA private key. > + /** > + * Generate key for PKCS#12 encryption, we use SHA1 with 1 iteration, as the > + * parameters used in PK11_ExportEncryptedPrivKeyInfo() above. Is it necessary to export the key as encrypted data and then decrypt it? Can't we just use one of the other PK11 export key functions? @@ +362,5 @@ > + // Generate key by SHA-1 > + nsresult rv; > + nsCOMPtr<nsICryptoHash> hash = > + do_CreateInstance("@mozilla.org/security/hash;1", &rv); > + if (rv != NS_OK) { if (NS_FAILED(rv)) @@ +367,5 @@ > + return SYSTEM_ERROR; > + } > + > + rv = hash->Init(nsICryptoHash::SHA1); > + if (rv != NS_OK) { if (NS_FAILED(rv)) @@ +678,2 @@ > > + if (strncmp(name, "WIFI_USERKEY_", 13)) { Again, flip this around and use !strncmp
Attachment #8456726 - Flags: review?(dkeeler) → review-
Comment on attachment 8460058 [details] [diff] [review] 0003. Support import PKCS12 certificate. V2 Review of attachment 8460058 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty reasonable. Just some nits. ::: dom/wifi/WifiCertService.cpp @@ +79,5 @@ > + } > + > + // Try import as PKCS#12 format. > + return ImportPkcs12Blob(buf, size, mPassword); > + nit: unnecessary blank line @@ +107,5 @@ > + { > + const nsAString::char_type *stringData = aString.BeginReading(); > + int len = aString.Length(); > + > +#ifdef IS_BIG_ENDIAN I think we can avoid this by first converting the nsAString to an nsACString (is the password always ASCII or UTF8? Use NS_ConvertUTF16toUTF8 or something) @@ +125,5 @@ > + nsresult ImportPkcs12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + // Get password. > + ScopedSECItem passwordItem( > + ::SECITEM_AllocItem(nullptr, nullptr, Not sure why you need the :: namespace indicator here... @@ +168,5 @@ > + // User certificate is always the first certificate. > + if (!userCert.get()) { > + userCert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), dip->der, > + NULL, false, true); > + continue; I would do: if (!userCert.get()) { ... } else { ... } instead of continue @@ +180,5 @@ > + if (!cert) { > + continue; > + } > + > + ImportCert(cert); Maybe this should be: if (cert) { ImportCert(cert); } @@ +196,5 @@ > + if (userCert.get()) { > + ImportCert(userCert); > + } > + > + return NS_OK; There isn't a lot of error checking/handling in this function. I'm pretty sure the PKCS12 bag could have nothing in it and this would return NS_OK, which seems strange to me. @@ +297,5 @@ > + nsCString nickname("WIFI_USERKEY_", 13); > + nickname += userNickname; > + > + ScopedSECItem nicknameItem( > + ::SECITEM_AllocItem(nullptr, nullptr, nickname.Length()) Not sure why you need the :: namespace indicator here... ::: security/build/nss.def @@ +46,5 @@ > CERT_DecodeCertFromPackage > CERT_DecodeCertificatePoliciesExtension > CERT_DecodeCertPackage > CERT_DecodeCRLDistributionPoints > +CERT_DecodeDERCertificate It doesn't look like this function is used in this patch.
Attachment #8460058 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #35) > Comment on attachment 8460058 [details] [diff] [review] > 0003. Support import PKCS12 certificate. V2 > > Review of attachment 8460058 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty reasonable. Just some nits. > > ::: dom/wifi/WifiCertService.cpp > @@ +107,5 @@ > > + { > > + const nsAString::char_type *stringData = aString.BeginReading(); > > + int len = aString.Length(); > > + > > +#ifdef IS_BIG_ENDIAN > > I think we can avoid this by first converting the nsAString to an nsACString > (is the password always ASCII or UTF8? Use NS_ConvertUTF16toUTF8 or > something) > I think it has to be 2-byte big-endian Unicode String, based on the PKCS 12 standard[1]. So transform Unicode to UTF8 won't do the work. I can't find any function could do this transform on nsAString, and nsPKCS12Blob also do the transform in this way[2] - that's where codes here are referenced from. [1] ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 [2] http://hg.mozilla.org/mozilla-central/file/a91ec42d6a9c/security/manager/ssl/src/nsPKCS12Blob.cpp#l415
(In reply to David Keeler (:keeler) [use needinfo?] from comment #35) > Comment on attachment 8460058 [details] [diff] [review] > 0003. Support import PKCS12 certificate. V2 > > Review of attachment 8460058 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty reasonable. Just some nits. > > ::: dom/wifi/WifiCertService.cpp > @@ +125,5 @@ > > + nsresult ImportPkcs12Blob(char* buf, uint32_t size, const nsAString& aPassword) > > + { > > + // Get password. > > + ScopedSECItem passwordItem( > > + ::SECITEM_AllocItem(nullptr, nullptr, > > Not sure why you need the :: namespace indicator here... > Without the :: namespace, the |SECITEM_AllocItem()| will be referenced to the function defined in [1] rather than in [2]. I can only fix this by adding prefix ::. [1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ScopedNSSTypes.h#257 [2] http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secitem.c#15
(In reply to David Keeler (:keeler) [use needinfo?] from comment #34) > Comment on attachment 8456726 [details] [diff] [review] > 0006. Support read private key in keystore. > > Review of attachment 8456726 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- because this seems unnecessary - see the 6th comment in this review. > > ::: ipc/keystore/KeyStore.cpp > @@ +344,5 @@ > > + > > + // Decrypt into RSA private key. > > + /** > > + * Generate key for PKCS#12 encryption, we use SHA1 with 1 iteration, as the > > + * parameters used in PK11_ExportEncryptedPrivKeyInfo() above. > > Is it necessary to export the key as encrypted data and then decrypt it? > Can't we just use one of the other PK11 export key functions? > I have tried |PK11_ExportDERPrivateKeyInfo()| but failed, and cause seems to be the private key imported from PKCS#12 doesn't have required attributes(CKA_PRIVATE_EXPONENT, CKA_PRIME_1, CKA_PRIME_2, etc.) to be exported[1]. This is the only workable way I found so far. [1] http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pk12.c#582
Flags: needinfo?(dkeeler)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #34) > Comment on attachment 8456726 [details] [diff] [review] > 0006. Support read private key in keystore. > > Review of attachment 8456726 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- because this seems unnecessary - see the 6th comment in this review. > > ::: ipc/keystore/KeyStore.cpp > @@ +138,5 @@ > > + if (strncmp(certName.string(), "WIFI_USERKEY_", 13)) { > > + return getCertificate(certName.string(), (const uint8_t **)item, > > + (int *)itemLength); > > + } > > + return getPrivateKey(certName.string(), (const uint8_t **)item, > > You shouldn't need to cast a uint8_t** to a const uint8_t** in this context > If I don't cast it, I will get compiler error like: invalid conversion from 'uint8_t** {aka unsigned char**}' to 'const uint8_t** {aka const unsigned char**}' [-fpermissive]
Attachment #8456723 - Flags: review?(vchang) → review+
Comment on attachment 8460058 [details] [diff] [review] 0003. Support import PKCS12 certificate. V2 Review of attachment 8460058 [details] [diff] [review]: ----------------------------------------------------------------- I am not familiar with NSS library, so I can't get you detail comments about that parts. I am wondering why do you create a new thread in WifiCertService::Start() but never using it. ::: dom/wifi/WifiCertService.cpp @@ +118,5 @@ > + } > +#endif > + // Make it null-terminated > + aItem->data[aItem->len - 1] = 0; > + aItem->data[aItem->len - 2] = 0; aItem->data is in unsigned char format, Do we need two zero for null-terminated?
Attachment #8460058 - Flags: review?(vchang)
Attachment #8456725 - Flags: review?(vchang) → review+
(In reply to Vincent Chang[:vchang] from comment #40) > Comment on attachment 8460058 [details] [diff] [review] > 0003. Support import PKCS12 certificate. V2 > > Review of attachment 8460058 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am not familiar with NSS library, so I can't get you detail comments about > that parts. > I am wondering why do you create a new thread in WifiCertService::Start() > but never using it. > > ::: dom/wifi/WifiCertService.cpp > @@ +118,5 @@ > > + } > > +#endif > > + // Make it null-terminated > > + aItem->data[aItem->len - 1] = 0; > > + aItem->data[aItem->len - 2] = 0; > > aItem->data is in unsigned char format, Do we need two zero for > null-terminated? Yes, PKCS 12 requires password to be two-byte big-endian format, with two terminating 0x00. [1] [1] ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1
(In reply to David Keeler (:keeler) [use needinfo?] from comment #34) > Comment on attachment 8456726 [details] [diff] [review] > 0006. Support read private key in keystore. > > Review of attachment 8456726 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- because this seems unnecessary - see the 6th comment in this review. > > ::: ipc/keystore/KeyStore.cpp > @@ +280,5 @@ > > return SUCCESS; > > } > > > > +static void > > +RC4(uint8_t *encrypted, int dataLen, uint8_t *key, int keyLen, uint8_t *decrypted) > > Don't implement RC4 yourself. Don't we have a library implementation? RC4_* functions are not exported from NSS, and I am still failing to do the same thing by PK11_Decrypt().
What's failing with PK11_Decrypt(). You shouldn't hardwire your crypto algorithm anyway. You want to be able to switch to new algorithms when old ones fails. NSS does export RC4 through it's standoard crypto interface (PK11_CreateContextXXXX() PK11_Op() PK11_DestroyContext()). bob
(In reply to Chuck Lee [:chucklee] from comment #36) > I think it has to be 2-byte big-endian Unicode String, based on the PKCS 12 > standard[1]. So transform Unicode to UTF8 won't do the work. > I can't find any function could do this transform on nsAString, and > nsPKCS12Blob also do the transform in this way[2] - that's where codes here > are referenced from. > > [1] ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > [2] > http://hg.mozilla.org/mozilla-central/file/a91ec42d6a9c/security/manager/ssl/ > src/nsPKCS12Blob.cpp#l415 Ok - this should at least be documented. Even better, why not share code between the two implementations? In fact, why not share more code with nsPKCS12Blob.cpp? (In reply to Chuck Lee [:chucklee] from comment #37) > Without the :: namespace, the |SECITEM_AllocItem()| will be referenced to > the function defined in [1] rather than in [2]. > I can only fix this by adding prefix ::. > > [1] > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > ScopedNSSTypes.h#257 > [2] > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secitem. > c#15 So why not use the one in ScopedNSSTypes.h? It checks for memory allocation failures, which is something this patch needs to do as well (remember, check for errors early and often). (In reply to Chuck Lee [:chucklee] from comment #38) > I have tried |PK11_ExportDERPrivateKeyInfo()| but failed, and cause seems to > be the private key imported from PKCS#12 doesn't have required > attributes(CKA_PRIVATE_EXPONENT, CKA_PRIME_1, CKA_PRIME_2, etc.) to be > exported[1]. > This is the only workable way I found so far. > > [1] > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/ > pk11pk12.c#582 When you call PK11_ImportEncryptedPrivateKeyInfo or PK11_ImportPrivateKeyInfo, I think if you change the second PR_TRUE to false, the imported key will be exportable unencrypted (that parameter is the "isPrivate" parameter - if that's set, then the key gets the "sensitive" attribute, which prevents it being exported unencrypted). It may be that something else sets the sensitive attribute, but that's a place to start looking. Also, remember to use nullptr in new code instead of NULL (similarly with true/false vs. PR_TRUE/PR_FALSE).
Flags: needinfo?(dkeeler)
(In reply to Robert Relyea from comment #43) > What's failing with PK11_Decrypt(). > > You shouldn't hardwire your crypto algorithm anyway. You want to be able to > switch to new algorithms when old ones fails. > > NSS does export RC4 through it's standoard crypto interface > (PK11_CreateContextXXXX() PK11_Op() PK11_DestroyContext()). > > bob CKR_MECHANISM_INVALID if I try to decrypt encrypted key by CKM_PBE_SHA1_RC4_40. No error if using CKM_RC4 but can't get correct result, still trying.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #44) > (In reply to Chuck Lee [:chucklee] from comment #36) > > I think it has to be 2-byte big-endian Unicode String, based on the PKCS 12 > > standard[1]. So transform Unicode to UTF8 won't do the work. > > I can't find any function could do this transform on nsAString, and > > nsPKCS12Blob also do the transform in this way[2] - that's where codes here > > are referenced from. > > Ok - this should at least be documented. Even better, why not share code > between the two implementations? In fact, why not share more code with > nsPKCS12Blob.cpp? > Actually there's nothing in common except the password transform function, and the implementation detail of this function is different in two files. I think there's not much code can be shared between the two implementations. > (In reply to Chuck Lee [:chucklee] from comment #37) > > Without the :: namespace, the |SECITEM_AllocItem()| will be referenced to > > the function defined in [1] rather than in [2]. > > I can only fix this by adding prefix ::. > > So why not use the one in ScopedNSSTypes.h? It checks for memory allocation > failures, which is something this patch needs to do as well (remember, check > for errors early and often). > It's not compatible with ScopedSECItem, so I think I'll stick with original function with more check. > (In reply to Chuck Lee [:chucklee] from comment #38) > > I have tried |PK11_ExportDERPrivateKeyInfo()| but failed, and cause seems to > > be the private key imported from PKCS#12 doesn't have required > > attributes(CKA_PRIVATE_EXPONENT, CKA_PRIME_1, CKA_PRIME_2, etc.) to be > > exported[1]. > > This is the only workable way I found so far. > > > > [1] > > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/ > > pk11pk12.c#582 > > When you call PK11_ImportEncryptedPrivateKeyInfo or > PK11_ImportPrivateKeyInfo, I think if you change the second PR_TRUE to > false, the imported key will be exportable unencrypted (that parameter is > the "isPrivate" parameter - if that's set, then the key gets the "sensitive" > attribute, which prevents it being exported unencrypted). It may be that > something else sets the sensitive attribute, but that's a place to start > looking. I tried and also get same result, the isPrivate seems only sets the attribute of the imported key but has no affect on other attributes. > > Also, remember to use nullptr in new code instead of NULL (similarly with > true/false vs. PR_TRUE/PR_FALSE). Thanks, I'll double check!
> CKR_MECHANISM_INVALID if I try to decrypt encrypted key by CKM_PBE_SHA1_RC4_40. > No error if using CKM_RC4 but can't get correct result, still trying. if you are trying to decrypt a pkcs12 blob, you need to call the pkcs12 functions. The blob itself tells the pkcs12 code what algorithm to use to decrypt. unfortunately pkcs12 is a bit obtuse (we don't have a nice simple wrapper for 'decrypt this pkcs #12 key'). The best sample code is nss/cmd/pk12util/pk12util.c
Address reviewer's comments.
Attachment #8460058 - Attachment is obsolete: true
Add reviewer's name
Attachment #8456723 - Attachment is obsolete: true
Address reviewer's comments.
Attachment #8456725 - Attachment is obsolete: true
1. Address reviewer's comments. 2. Use NSS API to perform RC4 decryption.
Attachment #8456726 - Attachment is obsolete: true
Attachment #8464442 - Flags: review?(dkeeler)
Comment on attachment 8464442 [details] [diff] [review] 0006. Support read private key in keystore. V2 Review of attachment 8464442 [details] [diff] [review]: ----------------------------------------------------------------- Ok - this looks reasonable. I didn't review for style nits, since this code appears to use a different style than what I'm used to working with. Watch out for unsigned/signed conversions. ::: ipc/keystore/KeyStore.cpp @@ +343,5 @@ > + > + hash->Update(DSP, sizeof(DSP)); > + > + nsCString hashResult; > + rv = hash->Finish(PR_FALSE, hashResult); false, not PR_FALSE @@ +354,5 @@ > + memcpy(key, hashResult.get(), sizeof(key)); > + > + // Decrypt by RC4 > + uint8_t *encryptedData = (uint8_t *)encryptedPrivateKey->encryptedData.data; > + int dataLen = encryptedPrivateKey->encryptedData.len; encryptedPrivateKey->encryptedData.len is an unsigned int, if I understand this correctly (encryptedData is a SECItem). Be careful about converting between unsigned and signed types. In particular, dataLen should really be of the same type. @@ +355,5 @@ > + > + // Decrypt by RC4 > + uint8_t *encryptedData = (uint8_t *)encryptedPrivateKey->encryptedData.data; > + int dataLen = encryptedPrivateKey->encryptedData.len; > + unsigned char *decryptedData = (unsigned char *)malloc(dataLen); Is malloc the allocator that should be used in this code?
Attachment #8464442 - Flags: review?(dkeeler) → review+
Address comment 52: 1. Change data types. 2. Use moz_malloc and moz_free. 3. Add more checks.
Attachment #8464442 - Attachment is obsolete: true
Comment on attachment 8460054 [details] [diff] [review] 0002. Allow read private key through PKCS12 decoder iterator. V2 Review of attachment 8460054 [details] [diff] [review]: ----------------------------------------------------------------- Bob's better to review this, but this is a r- from me. Among other things, please note the API of the existing SEC_PKCS12DecoderIterateNext, which ensures that the pointers on SEC_PKCS12DecoderItemStr are allocated appropriately, and thus freed while iterating. If you're introducing new pointers, you presumably want to bump reference counts / make copies / do something to ensure the memory safety. You can clearly see that the existing members (such as friendlyName or shroudAlg) are *already* available via decitem/bag, so if what you are doing was appropriate, we'd already be doing it.
Attachment #8460054 - Flags: superreview?(rrelyea)
Attachment #8460054 - Flags: review?(ryan.sleevi)
Attachment #8460054 - Flags: review-
(In reply to Ryan Sleevi from comment #55) > Comment on attachment 8460054 [details] [diff] [review] > 0002. Allow read private key through PKCS12 decoder iterator. V2 > > Review of attachment 8460054 [details] [diff] [review]: > ----------------------------------------------------------------- > > Bob's better to review this, but this is a r- from me. > > Among other things, please note the API of the existing > SEC_PKCS12DecoderIterateNext, which ensures that the pointers on > SEC_PKCS12DecoderItemStr are allocated appropriately, and thus freed while > iterating. If you're introducing new pointers, you presumably want to bump > reference counts / make copies / do something to ensure the memory safety. > You can clearly see that the existing members (such as friendlyName or > shroudAlg) are *already* available via decitem/bag, so if what you are doing > was appropriate, we'd already be doing it. I try to expose required info to import private key in PKCS#12 files with assigned nickname by |PK11_ImportEncryptedPrivateKeyInfo()|, as patch part 0003. This requires more info than current |decitem| provides, like slot, wincx, and most importantly, the key bag. I'll look into the memory safety part, but would like to know is my method appropriate for getting these information, or there's other method I can use. Thanks.
Reduce data structure and allocate memory for each exposed key info.
Attachment #8460054 - Attachment is obsolete: true
Attachment #8460054 - Flags: superreview?(rrelyea)
Attachment #8469794 - Flags: superreview?(rrelyea)
Attachment #8469794 - Flags: review?(ryan.sleevi)
Rebase, nss.def changes directory.
Attachment #8466012 - Attachment is obsolete: true
Rebase, nss.def changes directory.
Attachment #8466014 - Attachment is obsolete: true
Attachment #8456720 - Flags: review?(ekr)
Attachment #8456720 - Flags: review-
Attachment #8456720 - Flags: feedback?(rrelyea)
Attachment #8469794 - Flags: superreview?(rrelyea)
Attachment #8469794 - Flags: review?(ryan.sleevi)
Attachment #8469794 - Flags: review?(ekr)
Comment on attachment 8469794 [details] [diff] [review] 0002. Allow read private key through PKCS12 decoder iterator. V3 Review of attachment 8469794 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I am not familiar with this section of the NSS code. Given that rsleevi r-ed a previous version of this patch, you should ask him for re-review.
Attachment #8469794 - Flags: review?(ekr)
Comment on attachment 8456720 [details] [diff] [review] 0001. Fix nits in NSS. Review of attachment 8456720 [details] [diff] [review]: ----------------------------------------------------------------- This patch has already been r-ed by rsleevi. You should either engage with him or escalate to Bob/Wan-Teh.
Attachment #8456720 - Flags: review?(ekr)
rsleevi: I confirm that this appears to just be whitespace changes. I applied the patch and did 'hg diff -w' (In reply to Eric Rescorla (:ekr) from comment #61) > Comment on attachment 8456720 [details] [diff] [review] > 0001. Fix nits in NSS. > > Review of attachment 8456720 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch has already been r-ed by rsleevi. You should either engage with > him > or escalate to Bob/Wan-Teh.
Comment on attachment 8469794 [details] [diff] [review] 0002. Allow read private key through PKCS12 decoder iterator. V3 Chuck: I already suggested Bob as a reviewer, as did Eric. I don't think this is the right approach, as I've also already said, but I'll defer to Bob.
Attachment #8469794 - Flags: review?(ryan.sleevi) → review?(rrelyea)
Attachment #8456720 - Attachment is obsolete: true
Attachment #8469794 - Attachment is obsolete: true
Attachment #8469794 - Flags: review?(rrelyea)
Attachment #8495117 - Flags: review?(dkeeler)
Attachment #8495117 - Attachment is obsolete: true
Attachment #8495117 - Flags: review?(dkeeler)
Attachment #8495127 - Flags: review?(dkeeler)
Rename certificates after imported. So we don't have to modify NSS library.
Attachment #8469795 - Attachment is obsolete: true
Attachment #8495140 - Flags: review?(dkeeler)
Rebase and reorder.
Attachment #8464433 - Attachment is obsolete: true
Comment on attachment 8495127 [details] [diff] [review] 0001. Expose required NSS function. Review of attachment 8495127 [details] [diff] [review]: ----------------------------------------------------------------- This would be fine, but unfortunately PK11_SetObjectNickname isn't exported from NSS - see comment. ::: config/external/nss/nss.def @@ +413,5 @@ > PK11_ReferenceSlot > PK11_ResetToken > PK11SDR_Decrypt > PK11SDR_Encrypt > +PK11_SetObjectNickname This is actually a private function in NSS currently - we'll have to export it from NSS before using it externally. I think the best way to go about this would be to file a new bug in the NSS component and engage with the NSS developers to see how they would want this done.
Attachment #8495127 - Flags: review?(dkeeler) → review-
Comment on attachment 8495140 [details] [diff] [review] 0002. Support import PKCS12 certificate. Review of attachment 8495140 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly good, except a few things: a) Watch out for style nits: use 'Type* type', not 'Type *type' b) No c-style casts: use reinterpret_cast<Type*>(type) or whatever's appropriate c) Try to re-use/refactor code instead of copying it d) null-check any NSS objects before using them e) The thread-related changes seem unrelated to this bug ::: dom/wifi/WifiCertService.cpp @@ +20,5 @@ > #include "nsIWifiService.h" > #include "nsNetUtil.h" > #include "nsServiceManagerUtils.h" > #include "nsXULAppAPI.h" > +#include "pk11priv.h" I'm fairly sure pk11priv.h is meant to be private to NSS, and not used externally. @@ +102,5 @@ > + return ImportCert(cert); > + } > + > + static SECItem* > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) nit: use bool, not PRBool Also, avoid underscores in names: call this 'HandleNicknameCollision' or something @@ +104,5 @@ > + > + static SECItem* > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) > + { > + SECItem *newNick = new SECItem; nit: SECItem* newNick Also, don't use operator new for SECItem @@ +105,5 @@ > + static SECItem* > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) > + { > + SECItem *newNick = new SECItem; > + if (!newNick) nit: braces around the body of if statements @@ +110,5 @@ > + return nullptr; > + > + newNick->type = siAsciiString; > + // Dummy name, will be renamed later. > + newNick->data = (unsigned char*) strdup("Imported User Cert"); nit: no c-style casts @@ +124,5 @@ > + * this now. > + * > + * @see ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > + */ > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem *aItem) nit: nsAString&, SECItem* Also, why not refactor the code in nsPKCS12Blob::unicodeToItem and share it instead of copying the code? @@ +126,5 @@ > + * @see ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > + */ > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem *aItem) > + { > + const nsAString::char_type *stringData = aString.BeginReading(); nit: nsAString::char_type* @@ +143,5 @@ > + aItem->data[aItem->len - 1] = 0; > + aItem->data[aItem->len - 2] = 0; > + } > + > + nsresult ImportPkcs12Blob(char* buf, uint32_t size, const nsAString& aPassword) nit: PKCS12 @@ +153,5 @@ > + ); > + if (!passwordItem) { > + return NS_ERROR_OUT_OF_MEMORY; > + } > + nsStringToSECItem(aPassword, passwordItem); nsStringToSECItem should take care of allocating the SECItem, not the code that calls it (that way, it's easier to verify that there aren't any buffer overflows). @@ +166,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + // Assign data to decorder. > + SECStatus srv = SEC_PKCS12DecoderUpdate(p12dcx, (unsigned char *)buf, size); nit: c-style cast @@ +184,5 @@ > + return MapSECStatus(srv); > + } > + > + // Initialize slot. > + ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot()); nit: check for slot being null before using it @@ +204,5 @@ > + if (srv != SECSuccess) { > + return MapSECStatus(srv); > + } > + > + const SEC_PKCS12DecoderItem *dip; nit: SEC_PKCS12DecoderItem* @@ +211,5 @@ > + CopyUTF16toUTF8(mResult.mNickname, userNickname); > + while (SEC_PKCS12DecoderIterateNext(p12dcx, &dip) == SECSuccess) { > + if (dip->type == SEC_OID_PKCS12_V1_CERT_BAG_ID) { > + // Server CA and User CA. > + ScopedCERTCertificate cert( null-check cert before using it @@ +231,5 @@ > + continue; > + } > + > + char* nickname; > + uint32_t length; nit: declare and define length on the same line @@ +274,5 @@ > > return NS_OK; > } > > + nsresult ImportCert(CERTCertificate *aCert) nit: CERTCertificate* @@ +280,3 @@ > nsCString userNickname, fullNickname; > > + CopyUTF16toUTF8(mResult.mNickname, userNickname); Is it ever verified that mResult.mNickname actually is UTF-16? @@ -226,5 @@ > WifiCertService::Start(nsIWifiEventListener* aListener) > { > MOZ_ASSERT(aListener); > > - nsresult rv = NS_NewThread(getter_AddRefs(mRequestThread)); This seems unrelated to this bug and should be handled in its own bug.
Attachment #8495140 - Flags: review?(dkeeler) → review-
Comment on attachment 8495140 [details] [diff] [review] 0002. Support import PKCS12 certificate. Review of attachment 8495140 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for such quick and detailed review, I'll try to address every comments. There are some feedback I like to confirm. ::: dom/wifi/WifiCertService.cpp @@ +102,5 @@ > + return ImportCert(cert); > + } > + > + static SECItem* > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) This callback is defined to be PRBool. [1] [1] http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pkcs12/p12.h#40 @@ +104,5 @@ > + > + static SECItem* > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) > + { > + SECItem *newNick = new SECItem; I referenced it from nsPKCS12Blob.cpp[1], should it also get fixed? Is it correct to use PORT_Znew() instead as I found in pk12util.c[2]? [1] http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsPKCS12Blob.cpp#683 [2] http://dxr.mozilla.org/mozilla-central/source/security/nss/cmd/pk12util/pk12util.c#161 @@ +280,3 @@ > nsCString userNickname, fullNickname; > > + CopyUTF16toUTF8(mResult.mNickname, userNickname); mResult.mNickname and mResult.mPassword are passed from API of type DOMString, it should be UTF-16[1]. https://developer.mozilla.org/en/docs/Web/API/DOMString
Flags: needinfo?(dkeeler)
Comment on attachment 8495140 [details] [diff] [review] 0002. Support import PKCS12 certificate. Review of attachment 8495140 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +124,5 @@ > + * this now. > + * > + * @see ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > + */ > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem *aItem) I tried but nsPKCS12Blob doesn't expose as NSS API, it would take much work and review to expose a class use internally in NSS. I think I would keep this function now.
(In reply to Chuck Lee [:chucklee] from comment #72) > ::: dom/wifi/WifiCertService.cpp > @@ +102,5 @@ > > + return ImportCert(cert); > > + } > > + > > + static SECItem* > > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) > > This callback is defined to be PRBool. [1] Yeah, I was thinking PRBool was an alias for bool, but it's actually int, so let's keep it as PRBool here (although, in general, my understanding is we try not to use PR types in gecko). > @@ +104,5 @@ > > + > > + static SECItem* > > + nickname_collision(SECItem* aOldNickname, PRBool* aCancel, void* aWincx) > > + { > > + SECItem *newNick = new SECItem; > > I referenced it from nsPKCS12Blob.cpp[1], should it also get fixed? That would be nice. It can be done in another bug, though. > Is it correct to use PORT_Znew() instead as I found in pk12util.c[2]? Use SECITEM_AllocItem(nullptr, nullptr, <the length that the item is going to be>); > @@ +280,3 @@ > > nsCString userNickname, fullNickname; > > > > + CopyUTF16toUTF8(mResult.mNickname, userNickname); > > mResult.mNickname and mResult.mPassword are passed from API of type > DOMString, it should be UTF-16[1]. > > https://developer.mozilla.org/en/docs/Web/API/DOMString Great.
Flags: needinfo?(dkeeler)
(In reply to Chuck Lee [:chucklee] from comment #73) > ::: dom/wifi/WifiCertService.cpp > @@ +124,5 @@ > > + * this now. > > + * > > + * @see ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > > + */ > > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem *aItem) > > I tried but nsPKCS12Blob doesn't expose as NSS API, it would take much work > and review to expose a class use internally in NSS. > I think I would keep this function now. What you should be able to do is add nsPKCS12Blob.h to EXPORTS in security/manager/ssl/src/moz.build, and then #include "nsPKCS12Blob.h" and call unicodeToItem.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #75) > (In reply to Chuck Lee [:chucklee] from comment #73) > > ::: dom/wifi/WifiCertService.cpp > > @@ +124,5 @@ > > > + * this now. > > > + * > > > + * @see ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-12/pkcs-12v1.pdf, B.1 > > > + */ > > > + void nsStringToSECItem(const nsAString &aString, /*out*/ SECItem *aItem) > > > > I tried but nsPKCS12Blob doesn't expose as NSS API, it would take much work > > and review to expose a class use internally in NSS. > > I think I would keep this function now. > > What you should be able to do is add nsPKCS12Blob.h to EXPORTS in > security/manager/ssl/src/moz.build, and then #include "nsPKCS12Blob.h" and > call unicodeToItem. Thanks! I thought I can't expose these classes.
Attachment #8495127 - Attachment is obsolete: true
Attachment #8496778 - Flags: review?(dkeeler)
Address reviewer's comments.
Attachment #8495140 - Attachment is obsolete: true
Attachment #8496798 - Flags: review?(dkeeler)
Comment on attachment 8496778 [details] [diff] [review] 0001. Expose required NSS function. V2 Review of attachment 8496778 [details] [diff] [review]: ----------------------------------------------------------------- r=me given bug 1073330 goes as expected.
Attachment #8496778 - Flags: review?(dkeeler) → review+
Comment on attachment 8496798 [details] [diff] [review] 0002. Support import PKCS12 certificate. V2 Review of attachment 8496798 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. r=me with comments addressed. On another note, remember that it's only things in security/nss that are in NSS (and are thus more work to change). Things in security/manager and security/apps are part of PSM and can be changed if necessary. Lastly, where are the tests for this? It seems like we should make sure this infrastructure works as expected. ::: dom/wifi/WifiCertService.cpp @@ +112,5 @@ > + } > + > + newNick->type = siAsciiString; > + // Dummy name, will be renamed later. > + memcpy(newNick->data, dummyName, strlen(dummyName)); Since we're sort-of treating newNick as a null-terminated string, it would be good to ensure it gets null-terminated. @@ +113,5 @@ > + > + newNick->type = siAsciiString; > + // Dummy name, will be renamed later. > + memcpy(newNick->data, dummyName, strlen(dummyName)); > + newNick->len = strlen(dummyName); nit: unnecessary extra space after len @@ +120,5 @@ > + } > + > + nsresult ImportPKCS12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + SECItem passwordItem; This should be a ScopedSECItem (otherwise this leaks memory) @@ +136,5 @@ > + } > + > + // Assign data to decorder. > + SECStatus srv = SEC_PKCS12DecoderUpdate(p12dcx, > + reinterpret_cast<unsigned char *>(buf), nit: 'unsigned char*' ::: security/manager/ssl/src/moz.build @@ +13,5 @@ > 'ScopedNSSTypes.h', > ] > > +# For exporting nsPKCS12Blob > +EXPORTS += [ These should all be in the same list, up at the top. ::: security/manager/ssl/src/nsPKCS12Blob.h @@ +39,5 @@ > nsresult ImportFromFile(nsIFile *file); > > // PKCS#12 Export > nsresult ExportToFile(nsIFile *file, nsIX509Cert **certs, int numCerts); > + static void unicodeToItem(const char16_t *, SECItem *); nit: I would prefer a capital "U" here ("UnicodeToItem")
Attachment #8496798 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #80) > Comment on attachment 8496798 [details] [diff] [review] > 0002. Support import PKCS12 certificate. V2 > > Review of attachment 8496798 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good. r=me with comments addressed. On another note, remember > that it's only things in security/nss that are in NSS (and are thus more > work to change). Things in security/manager and security/apps are part of > PSM and can be changed if necessary. > Lastly, where are the tests for this? It seems like we should make sure this > infrastructure works as expected. > Thanks for these help! I'll try if I can run unit test on the service. If not, then I have to add test in marionette test along with API support, bug 1049476.
Address NSS related parts per comment 80. Hi Vincent, please help review the tiny change in WifiWorker, thanks.
Attachment #8496798 - Attachment is obsolete: true
Attachment #8499422 - Flags: review?(vchang)
(In reply to Chuck Lee [:chucklee] from comment #81) > (In reply to David Keeler (:keeler) [use needinfo?] from comment #80) > > Comment on attachment 8496798 [details] [diff] [review] > > 0002. Support import PKCS12 certificate. V2 > > > > Review of attachment 8496798 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This looks good. r=me with comments addressed. On another note, remember > > that it's only things in security/nss that are in NSS (and are thus more > > work to change). Things in security/manager and security/apps are part of > > PSM and can be changed if necessary. > > Lastly, where are the tests for this? It seems like we should make sure this > > infrastructure works as expected. > > > > Thanks for these help! > I'll try if I can run unit test on the service. > If not, then I have to add test in marionette test along with API support, > bug 1049476. I think run test in API level is more straightforward, I'll open another bug for the certificate tests.
Comment on attachment 8499422 [details] [diff] [review] 0002. Support import PKCS12 certificate. V3. r=dkeeler Review of attachment 8499422 [details] [diff] [review]: ----------------------------------------------------------------- WifiWorker.js part looks good.
Attachment #8499422 - Flags: review?(vchang) → review+
Add reviewer's name.
Attachment #8496778 - Attachment is obsolete: true
Treat deleting non-existing certificate as successful.
Attachment #8495142 - Attachment is obsolete: true
Comment on attachment 8500276 [details] [diff] [review] 0002. Support import PKCS12 certificate. V3. r=dkeeler, vchang Review of attachment 8500276 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +121,5 @@ > + } > + > + nsresult ImportPKCS12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + ScopedSECItem passwordItem(::SECITEM_AllocItem(nullptr, nullptr, 0)); Hi david, I found that using ScopedSECItem here will cause b2g crash about 80% of the time(but won't crash anymore after first crash), while using SECItem won't. Does is safe to use ScopedSECItem in CryptoTask? Maybe it's better to use SECItem and release allocated memory before each return?
Flags: needinfo?(dkeeler)
Comment on attachment 8500276 [details] [diff] [review] 0002. Support import PKCS12 certificate. V3. r=dkeeler, vchang Review of attachment 8500276 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +121,5 @@ > + } > + > + nsresult ImportPKCS12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + ScopedSECItem passwordItem(::SECITEM_AllocItem(nullptr, nullptr, 0)); Yeah, looks like nsPKCS12Blob::UnicodeToItem isn't really set up to do this properly (it should return a SECItem* rather than taking a SECItem* and allocating memory to it). Would you mind filing a follow-up bug to fix this? In the meantime, go ahead and use a stack-allocated SECItem and SECITEM_FreeItem(&passwordItem, false); where appropriate.
Flags: needinfo?(dkeeler)
Are you waiting for the enhancement from bug 1073330 to be available in mozilla-central? Then we'll have to tag a beta of NSS and land into m-c.
(In reply to Kai Engert (:kaie) from comment #91) > Are you waiting for the enhancement from bug 1073330 to be available in > mozilla-central? > Then we'll have to tag a beta of NSS and land into m-c. Yes, I hope bug 1073330 can be landed to m-c before November. So we might have to land a beta of NSS to m-c if 3.18 won't be released recently. Really appreciated for you help on NSS!
I found that get private key through keystore protocol has been removed in frame and changed to use public key + sign for certification, I'll try implement corresponding part in gecko's keystore.
Add |getPublickey()| and |signData()| to support function call to |get_pubkey()| and |sign()| in keystore, which is used in openssl-engine-style private key authentication. Tested with WPA-EAP-TLS AP using freeradius.
Attachment #8495144 - Attachment is obsolete: true
Attachment #8510787 - Flags: review?(dkeeler)
Depends on: 1088969
Comment on attachment 8510787 [details] [diff] [review] 0005. Support read private key in keystore. V6 Review of attachment 8510787 [details] [diff] [review]: ----------------------------------------------------------------- In general, this looks reasonable. However, I'm not familiar enough with the ipc code to be a good reviewer for some of this. I noted a few specific issues with the use of NSS functions. Also, more generally, how does this code ensure that NSS has been initialized and remains available while it's using it? (See e.g. the documentation in https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSShutDown.h#167 ). This might require some re-working, so r- for now. ::: ipc/keystore/KeyStore.cpp @@ +386,5 @@ > + > + // Export private key in PKCS#12 encrypted format, no password. > + unsigned char pwstr[] = {0, 0}; > + SECItem password = {siBuffer, pwstr, sizeof(pwstr)}; > + SECKEYEncryptedPrivateKeyInfo *encryptedPrivateKey = This should probably be a scoped type (I believe it leaks as-is). @@ +396,5 @@ > + return KEY_NOT_FOUND; > + } > + > + // Decrypt into RSA private key. > + /** nit: use C++-style comments (//) @@ +406,5 @@ > + memset(DSP, 0x01, 64); // Diversifier part, ID = 1 for decryption. > + memset(DSP + 128, 0x00, 64); // Password part, no password. > + > + uint8_t *S = &DSP[64], // Salt part. > + *salt = encryptedPrivateKey->algorithm.parameters.data + 4; nit: use full declarations for each variable: uint8_t *S = &DSP[64]; uint8_t *salt = encryptedPrivateKey...; @@ +439,5 @@ > + memcpy(key, hashResult.get(), sizeof(key)); > + > + // Decrypt by RC4 > + uint8_t *encryptedData = (uint8_t *)encryptedPrivateKey->encryptedData.data; > + unsigned int dataLen = encryptedPrivateKey->encryptedData.len; I think you actually need to get dataLen by calling PK11_Decrypt with a null pointer in the place of the decryptedData pointer and then calling it again to actually decrypt the data (if I'm reading the code correctly: https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#1496 ). @@ +440,5 @@ > + > + // Decrypt by RC4 > + uint8_t *encryptedData = (uint8_t *)encryptedPrivateKey->encryptedData.data; > + unsigned int dataLen = encryptedPrivateKey->encryptedData.len; > + unsigned char *decryptedData = (unsigned char *)moz_malloc(dataLen); Can we make this scoped? If any of the later error cases are encountered, this will leak. @@ +451,5 @@ > + return SYSTEM_ERROR; > + } > + SECItem keyItem = {siBuffer, key, sizeof(key)}; > + ScopedPK11SymKey symKey(PK11_ImportSymKey(slot, CKM_RC4, PK11_OriginUnwrap, > + CKA_DECRYPT, &keyItem, nullptr)); nit: this should be indented to line up with 'slot' on the previous line @@ +550,5 @@ > + * Hash data length for TLS is 36 bytes, incoming data have pading in front of > + * hash value, in the form of > + * 00 01 FF FF FF FF FF FF > + * FF FF FF FF FF FF FF FF > + * .... Is it important to verify that the incoming data is actually of this form? Signature verification schemes have been subverted by not enforcing things like this (see e.g. http://csrc.nist.gov/groups/ST/toolkit/documents/dss/RSAstatement_10-12-06.pdf ) ::: ipc/keystore/KeyStore.h @@ +43,5 @@ > + size_t *aKeyDataLength); > +ResponseCode getPublicKey(const char *aKeyName, const uint8_t **aKeyData, > + size_t *aKeyDataLength); > +ResponseCode signData(const char *aKeyName, const uint8_t* data, size_t length, > + uint8_t** out, size_t* outLength); nit: be consistent about where the '*' goes - looks like the style in this file is to have it on the right, not the left.
Attachment #8510787 - Flags: review?(dkeeler) → review-
> @@ +550,5 @@ > > + * Hash data length for TLS is 36 bytes, incoming data have pading in front of > > + * hash value, in the form of > > + * 00 01 FF FF FF FF FF FF > > + * FF FF FF FF FF FF FF FF > > + * .... > > Is it important to verify that the incoming data is actually of this form? > Signature verification schemes have been subverted by not enforcing things > like this (see e.g. > http://csrc.nist.gov/groups/ST/toolkit/documents/dss/RSAstatement_10-12-06. > pdf ) Hi David, Thanks for all these details, just some questions about the signature. Based on the document, I can check if the padding exist first, then use data in ASN.1 to check if data in hash part is correct? But what can I do if the ASN.1 part is also modified?
I'm not quite sure what you're asking here. It doesn't look like there's any ASN.1 to decode/verify - I was just saying you should make sure the input is of the form you're expecting and return an error if it isn't.
Add |SECKEY_DestroyEncryptedPrivateKeyInfo|.
Attachment #8510783 - Attachment is obsolete: true
Attachment #8513259 - Flags: review?(dkeeler)
Address comment 96 including: 1. Add NSSShutdown check and protection. 2. Use Scoped pointers. 3. Check hash content for TLS sign. 4. Fix nits.
Attachment #8510787 - Attachment is obsolete: true
Attachment #8513261 - Flags: review?(dkeeler)
Comment on attachment 8513259 [details] [diff] [review] 0001. Expose required NSS function. V3 Review of attachment 8513259 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. However, in the future I think it would be helpful to have any changes to this file appear in the patch that adds the dependency on the additional function(s). That way, it's easy to see that the additions are necessary and correct.
Attachment #8513259 - Flags: review?(dkeeler) → review+
Comment on attachment 8513261 [details] [diff] [review] 0005. Support read private key in keystore. V7 Review of attachment 8513261 [details] [diff] [review]: ----------------------------------------------------------------- The use of NSS/PSM functionality looks good. I noted a few style nits and places where it would be good to do more bounds checking, etc. r=me with those addressed. I still think it would be good to also get review from someone else who has worked with this code (i.e. ipc/keystore code). ::: ipc/keystore/KeyStore.cpp @@ +161,5 @@ > } > > // Provide service for binder. > +class KeyStoreService: public BnKeystoreService, > + public nsNSSShutDownObject nit: I think it's common to have the ',' from the previous line here on the second line under the ':' (and also to have a space before the ':') @@ +437,5 @@ > + uint8_t *S = &DSP[64]; // Salt part. > + uint8_t *salt = encryptedPrivateKey->algorithm.parameters.data + 4; > + int saltLength = (int)encryptedPrivateKey->algorithm.parameters.data[3]; > + for (int i = 0; i < 64; i++) { > + S[i] = salt[i % saltLength]; We should ensure that encryptedPrivateKey->algorithm.parameters.data is long enough for the data being read out of it and that saltLength isn't negative. @@ +453,5 @@ > + if (NS_FAILED(rv)) { > + return SYSTEM_ERROR; > + } > + > + hash->Update(DSP, sizeof(DSP)); nit: check the return value of Update @@ +504,5 @@ > + } > + > + // Export key in PEM format. > + char *keyPEM = nullptr; > + keyPEM = PL_Base64Encode((const char *)decryptedData->data, decryptedDataLen, nit: no need to declare and initialize on separate lines - just do it all at once @@ +562,5 @@ > + return SUCCESS; > +} > + > +ResponseCode signData(const char *aKeyName, const uint8_t *data, size_t length, > + uint8_t** out, size_t* outLength) nit: the placement of *s is still a bit inconsistent in general @@ +592,5 @@ > + // > + uint8_t *hash = (uint8_t *)data; > + if (hash[0] == 0x00 && hash[1] == 0x01 && hash[2] == 0xFF && hash[3] == 0xFF) { > + hash += 4; > + while (*hash == 0xFF) { hash++; } Be sure to do bounds checking when accessing hash/data (i.e. hash must always be less than data + length) @@ +599,5 @@ > + } > + hash++; > + } > + const int HASH_LENGTH = 36; > + if ((size_t)(hash + HASH_LENGTH - data) > length) { Shouldn't it be the case that hash + HASH_LENGTH == data + length? @@ +615,5 @@ > + > + SECStatus srv; > + srv = PK11_Sign(privateKey, &signItem, &hashItem); > + if (srv != SECSuccess) { > + return SYSTEM_ERROR; In the error case, signBuf will be leaked here. How about using a ScopedSECItem? If signing succeeds, you can do the following: *out = signItem->data; *outLength = signItem->len; signItem.forget(); (or .release() - I forget which it is) ::: ipc/keystore/KeyStore.h @@ +101,5 @@ > nsAString& aAddrStr); > }; > > +class KeyStore : public mozilla::ipc::UnixSocketConsumer, > + public nsNSSShutDownObject nit: I think it's common to have the ',' from the previous line here on the second line under the ':'
Attachment #8513261 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #101) > Comment on attachment 8513259 [details] [diff] [review] > 0001. Expose required NSS function. V3 > > Review of attachment 8513259 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine. However, in the future I think it would be helpful to have > any changes to this file appear in the patch that adds the dependency on the > additional function(s). That way, it's easy to see that the additions are > necessary and correct. Thanks! Since takes almost no work to make the patch clear, I will apply the change right now.
> @@ +615,5 @@ > > + > > + SECStatus srv; > > + srv = PK11_Sign(privateKey, &signItem, &hashItem); > > + if (srv != SECSuccess) { > > + return SYSTEM_ERROR; > > In the error case, signBuf will be leaked here. How about using a > ScopedSECItem? If signing succeeds, you can do the following: > > *out = signItem->data; > *outLength = signItem->len; > signItem.forget(); (or .release() - I forget which it is) > It seems that release()[2] will leak whole secitem for preserveing signItem->data, I think it will do what getPublicKey() does, malloc another memory and copy content if signData->item into it, then return it for |out|. Thanks for these review iterations, it's nice to work with you! [2] http://dxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/ScopedPtr.h#60
Address comment 101: move change of nss.def into corresponding patch.
Attachment #8504485 - Attachment is obsolete: true
Attachment #8513259 - Attachment is obsolete: true
Address comment 101: move change of nss.def into corresponding patch and comment 102.
Attachment #8513261 - Attachment is obsolete: true
Comment on attachment 8514821 [details] [diff] [review] 0004. Support read private key in keystore. V8. r=dkeeler qdot is not available. Since keystore implements unixsocket, review from unixsocket peer should be helpful.
Attachment #8514821 - Flags: review?(btian)
Comment on attachment 8514821 [details] [diff] [review] 0004. Support read private key in keystore. V8. r=dkeeler Change reviewer as Ben suggested.
Attachment #8514821 - Flags: review?(btian) → review?(tzimmermann)
Comment on attachment 8514821 [details] [diff] [review] 0004. Support read private key in keystore. V8. r=dkeeler Review of attachment 8514821 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I'm not qualified to review this code.
Attachment #8514821 - Flags: review?(tzimmermann)
Comment on attachment 8514821 [details] [diff] [review] 0004. Support read private key in keystore. V8. r=dkeeler Hi Kyle, NSS-related codes already reviewed by David, please check if you have any concerns on other parts. Thanks!
Attachment #8514821 - Flags: review?(kyle)
Update code based on improvements from bug 1094177.
Attachment #8514821 - Attachment is obsolete: true
Attachment #8514821 - Flags: review?(kyle)
Attachment #8518748 - Flags: review?(kyle)
I see that you haven't yet checked in these patches, and that mozilla-central isn't yet using the PK11_SetCertificateNickname API. I believe the implementation of PK11_SetCertificateNickname is incomplete, I found at least one bug. Given that tomorrow is the merge date, ending the current development phase, and given that it's thanksgiving in the US, it seems unlikely that you'll complete this work today. Therefore I'd like to backout the PK11_SetCertificateNickname feature from NSS and mozilla-central, to avoid having the buggy implementation merged over to the aurora branch. Please let's continue the discussion in bug 1073330 (and maybe let's even discuss if you could potentially use the right nickname at import time, avoiding the need to change it later.)
Comment on attachment 8518748 [details] [diff] [review] 0004. Support read private key in keystore. V9. r=dkeeler Review of attachment 8518748 [details] [diff] [review]: ----------------------------------------------------------------- What little I recognize of this looks ok to me, but I'm pretty sure you're ok not asking me for reviews anymore. This has changed well beyond the stuff that I was looking at when it started, and I also don't maintain UnixSocket anymore.
Attachment #8518748 - Flags: review?(kyle) → review+
Is firefox 37 an acceptable target for this bug?
(In reply to Kai Engert (:kaie) from comment #118) > Is firefox 37 an acceptable target for this bug? (as opposed to firefox 36 - this gives us a bit more time to implement a better fix in NSS.) And another question, out of curiousity: Why do you need the different nicknames? Which code will handle the certificates differently based on their nicknames, and what effect will it have? Will you manipulate any code that makes decisions about how to trust the certificates? Will you import the wifi CA cert with flags set to trusted, and might that have side effects to the global trust list?
No longer depends on: 1088969
(In reply to Kai Engert (:kaie) from comment #119) > (In reply to Kai Engert (:kaie) from comment #118) > > Is firefox 37 an acceptable target for this bug? > > (as opposed to firefox 36 - this gives us a bit more time to implement a > better fix in NSS.) I think firefox 37 is acceptable, but I am not the one managing the requirements and schedules. I would leave the release target question to Vincent.
Flags: needinfo?(vchang)
I thinks it's better to answer these questions one by one, so I put them inline. (In reply to Kai Engert (:kaie) from comment #119) > And another question, out of curiousity: Why do you need the different > nicknames? > Which code will handle the certificates differently based on their > nicknames, and what effect will it have? For user side, we allow user to give nickname for imported certs, so it's easier for user to select which one to use for certain wifi network API. In Gecko side, we apply a naming rule for nickname of certs imported from wifi(WIFI_SERVERCERT_ prefix For CA cert, and WIFI_USERCERT_ prefix for user cert). This makes import code simpler and we don't have to manage another database to mapping user assigned nickname to NSS nickname. Also this makes list function easier because we can generate list based on nicknames, and ignore built-in CAs. [1] Also gonk requires specific name of each certificate and private key[2], using naming rule can also preventing other privatekeys being read from NSS. [1] https://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#3474 [2] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1012549&attachment=8518748, getPrivateKey() > Will you manipulate any code that makes decisions about how to trust > the certificates? > Will you import the wifi CA cert with flags set to trusted, and might > that have side effects to the global trust list? No, I set certificate trust to NULL while importing. If my understandings are correct, these certificates should not appear in the global trust list. But the trust flag seems to be set automatically while importing PKCS12, while I can't find an API to change trust flag of certifcates. [3] https://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiCertService.cpp#157
Comment on attachment 8514811 [details] [diff] [review] 0001. Support import PKCS12 certificate. V5. r=dkeeler. r=vchang. Review of attachment 8514811 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +123,5 @@ > + nsresult ImportPKCS12Blob(char* buf, uint32_t size, const nsAString& aPassword) > + { > + SECItem passwordItem; > + nsString password(aPassword); > + nsPKCS12Blob::UnicodeToItem(password.get(), &passwordItem); I think we can use mozilla::NativeEndian::swapToBigEndianInPlace here instead of nsPKCS12Blob::UnicodeToItem. Then, nothing in passwordItem needs to be dynamically-allocated - it can just point to the resulting data.
Comment on attachment 8562061 [details] [diff] [review] 0001. Support import PKCS12 certificate. V6. r=dkeeler. r=vchang. Review of attachment 8562061 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +167,5 @@ > + } > + > + mozilla::NativeEndian::copyAndSwapToBigEndian(passwordItem->data, > + password.BeginReading(), > + length); Hi David, |mozilla::NativeEndian::swapToBigEndianInPlace()| you mentioned do work. most of the code are similar: mozilla::NativeEndian::swapToBigEndianInPlace( password.BeginWriting(), length); SECItem passwordItem = { siBuffer, (unsigned char *)password.BeginWriting(), length * sizeof(nsString::char_type)) }; But I felt a bit uncomfortable about manipulating memory of nsString directly, although I know it should be OK here. So in this patch, I still allocates memory for password and use |copyAndSwapToBigEndian()| instead. I like to know you opinion, thanks!
Comment on attachment 8562061 [details] [diff] [review] 0001. Support import PKCS12 certificate. V6. r=dkeeler. r=vchang. Review of attachment 8562061 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/WifiCertService.cpp @@ +159,5 @@ > + // passwordItem is required to be big-endian form of password, stored in char > + // array, including the null-termination. > + uint32_t length = password.Length() + 1; > + ScopedSECItem passwordItem( > + ::SECITEM_AllocItem(nullptr, nullptr, length * sizeof(nsString::char_type))); I don't think SECITEM_AllocItem is guaranteed to zero the memory in the item, so it might be a good idea to do that for at least the last two bytes. @@ +167,5 @@ > + } > + > + mozilla::NativeEndian::copyAndSwapToBigEndian(passwordItem->data, > + password.BeginReading(), > + length); Good call - it's probably not safe to modify the internal memory of nsString. I think this is a good approach.
Attachment #8562061 - Flags: feedback?(dkeeler) → feedback+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #125) > Comment on attachment 8562061 [details] [diff] [review] > 0001. Support import PKCS12 certificate. V6. r=dkeeler. r=vchang. > > Review of attachment 8562061 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/wifi/WifiCertService.cpp > @@ +159,5 @@ > > + // passwordItem is required to be big-endian form of password, stored in char > > + // array, including the null-termination. > > + uint32_t length = password.Length() + 1; > > + ScopedSECItem passwordItem( > > + ::SECITEM_AllocItem(nullptr, nullptr, length * sizeof(nsString::char_type))); > > I don't think SECITEM_AllocItem is guaranteed to zero the memory in the > item, so it might be a good idea to do that for at least the last two bytes. > nsString is a null-terminated string, by setting |length = password.Length() + 1|, I think the trailing zeros of password will be copied to passwordItem make it also null-terminated.
No longer depends on: 1073330
Update callback function based on final NSS API definition per bug 1113453. This bug is good to go after NSS 3.18 is released and merged into gecko
Attachment #8562061 - Attachment is obsolete: true
Fix build error "error: comparison between signed and unsigned integer expressions"
Attachment #8562481 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b5a6c5e5a0 Latest try seems OK, the intermittent failures seems irrelevant to the patches.
Keywords: checkin-needed
Flags: needinfo?(vchang)
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: