Closed Bug 1304587 Opened 7 years ago Closed 7 years ago

Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.
58 bytes, text/x-review-board-request
keeler
: review+
Details 
Typically, the interfaces involved don't need to use raw char/char16_t strings, and hence can benefit from the additional safety of using the Mozilla string classes.

Note: Changes that break compat with existing JS code is out of scope for this bug.
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

https://reviewboard.mozilla.org/r/80452/#review79492

Looks good to me. I've made some suggestions for further changes, and I'd like to see what those look like, so I'll just clear the review for now. If we decide not to go in that direction, that's fine, though.

::: security/manager/ssl/nsIPK11Token.idl:62
(Diff revision 1)
> +   * @param password The password to check.
> +   * @return true if the password was correct, false otherwise.
> +   */
> +  boolean checkPassword(in AUTF8String password);
> +  void initPassword(in AUTF8String initialPassword);
>    void changePassword(in wstring oldPassword, in wstring newPassword);

Can we make these AString?

::: security/manager/ssl/nsIPKCS11Module.idl:16
(Diff revision 1)
>  
>  [scriptable, uuid(8a44bdf9-d1a5-4734-bd5a-34ed7fe564c2)]
>  interface nsIPKCS11Module : nsISupports
>  {
> -  readonly attribute wstring name;
> +  readonly attribute AUTF8String name;
>    readonly attribute wstring libName;

Similarly, can this be AString?

::: security/manager/ssl/nsPK11TokenDB.cpp:52
(Diff revision 1)
>      return rv;
>    }
>  
>    // Set the Label field
>    const char* ccLabel = mozilla::BitwiseCast<char*, CK_UTF8CHAR*>(tokInfo.label);
> -  const nsACString& cLabel = Substring(
> +  // XXX: If/when all our supported compilers provide strnlen() we can stop

Maybe let's say "TODO" instead of "XXX".

::: security/manager/ssl/nsPK11TokenDB.cpp:112
(Diff revision 1)
>  NS_IMETHODIMP
> -nsPK11Token::GetTokenName(char16_t** aTokenName)
> +nsPK11Token::GetTokenName(/*out*/ nsACString& tokenName)
>  {
> -  NS_ENSURE_ARG_POINTER(aTokenName);
> -
>    nsNSSShutDownPreventionLock locker;

The next ~10 lines are pretty boiler-platey. I wonder if it would be worth factoring this out?

::: security/manager/ssl/nsPKCS11Slot.cpp:125
(Diff revision 1)
>  NS_IMETHODIMP
> -nsPKCS11Slot::GetDesc(char16_t** aDesc)
> +nsPKCS11Slot::GetDesc(/*out*/ nsACString& desc)
>  {
> -  NS_ENSURE_ARG_POINTER(aDesc);
> -
>    nsNSSShutDownPreventionLock locker;

Similar boiler-plate code issue here...

::: security/manager/ssl/nsPKCS12Blob.cpp:175
(Diff revision 1)
>  
> -  mToken->GetTokenName(getter_Copies(tokenName));
> +  rv = mToken->GetTokenName(tokenName);
> +  if (NS_FAILED(rv)) {
> +    goto finish;
> +  }
>    {

I think we can get rid of this extra scope now.
Attachment #8793826 - Flags: review?(dkeeler)
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

https://reviewboard.mozilla.org/r/80452/#review79492

Thanks for the suggestions - turns out it just involved knowing which methods to call.

> Can we make these AString?

As it turns out, yes.

C++ code can't detect or reflect to JS null or undefined args that have the IDL types AString, ACString, and so on using just the usual ```Assign()```, ```operator=``` etc - they just become the empty string once the boundary is crossed.
But, this can actually be accomplished via the ```IsVoid()``` and ```SetIsVoid(bool)``` methods.

So, I've changed these params to AUTF8String instead (not AString, for the reason I noted in the commit message).

> Similarly, can this be AString?

Same as above.

> Maybe let's say "TODO" instead of "XXX".

Done. I filed a corresponding bug as well.

> The next ~10 lines are pretty boiler-platey. I wonder if it would be worth factoring this out?

Done.

> Similar boiler-plate code issue here...

Also done.
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

(Cancelling review because I want to go through the changes here to make sure I'm not making the same mistake that lead to Bug 1305531.)

Sorry for the spam.
Attachment #8793826 - Flags: review?(dkeeler)
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

I went through the changes again and I believe they're fine, but I can use AString instead of AUTF8String to reduce the risk if you prefer.
Attachment #8793826 - Flags: review?(dkeeler)
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

https://reviewboard.mozilla.org/r/80452/#review80848

Looks good to me. I'm fairly sure we won't have the same problem that led to bug 1305531.

::: security/manager/ssl/nsPK11TokenDB.cpp:353
(Diff revisions 1 - 2)
>    if (isAlreadyShutDown())
>      return NS_ERROR_NOT_AVAILABLE;
>  
> -  NS_ConvertUTF16toUTF8 utf8OldPassword(oldPassword);
> -  NS_ConvertUTF16toUTF8 utf8NewPassword(newPassword);
> -
> +  // PK11_ChangePW() has different semantics for the empty string and for
> +  // nullptr. In order to support this difference, we need to check IsVoid() to
> +  // find out whether our caller supplied null or undefined args.

I might be misunderstanding, but I think this last bit is a little unclear. Maybe "... we need to check IsVoid() to find out if our caller supplied null/undefined args or just empty strings" or something?

::: security/manager/ssl/nsIPK11Token.idl:62
(Diff revision 2)
> +   * @param password The password to check.
> +   * @return true if the password was correct, false otherwise.
> +   */
> +  boolean checkPassword(in AUTF8String password);
> +  void initPassword(in AUTF8String initialPassword);
> +  void changePassword(in AUTF8String oldPassword, in AUTF8String newPassword);

For changePassword, it looks like we don't have a testcase where newPassword is a string with codepoints outside the 00-ff range (it looks like oldPassword, checkPassword and initPassword do have coverage).

::: security/manager/ssl/nsIPKCS11Module.idl:18
(Diff revision 2)
>  interface nsIPKCS11Module : nsISupports
>  {
> -  readonly attribute wstring name;
> -  readonly attribute wstring libName;
> +  readonly attribute AUTF8String name;
> +  readonly attribute AUTF8String libName;
>  
> -  nsIPKCS11Slot findSlotByName(in wstring name);
> +  nsIPKCS11Slot findSlotByName(in AUTF8String name);

I wonder if it would be a good idea to add a slot to the test module with a name with high codepoints... that can probably be a follow-up, though.
Attachment #8793826 - Flags: review?(dkeeler) → review+
Comment on attachment 8793826 [details]
Bug 1304587 - Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files.

https://reviewboard.mozilla.org/r/80452/#review80848

Thanks!

> I might be misunderstanding, but I think this last bit is a little unclear. Maybe "... we need to check IsVoid() to find out if our caller supplied null/undefined args or just empty strings" or something?

Yup, your wording is better.

> I wonder if it would be a good idea to add a slot to the test module with a name with high codepoints... that can probably be a follow-up, though.

I filed Bug 1306632.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e8bda9b5b8b8
Avoid using types that correspond to char/char16_t strings in PKCS #11 IDL files. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8bda9b5b8b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1306930
You need to log in before you can comment on or make changes to this bug.