1224139 - PK11 unchecked error values

Status: REOPENED

(NSS :: Libraries, defect, P3)

Description

[Franziskus Kiefer [:franziskus]]

Calls to PK11_Authenticate (CID 221844, 221845, 221862, 221863) and PK11_GetAttributes (CID 221846) sometimes miss checks of their return value.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: checking error values

Checking error values from PK11 functions.

I'm not exactly sure about the error codes here and either don't set any or SECFailure.

Patch is also at https://codereview.appspot.com/273350043

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8686538 [details] [diff] [review]
checking error values

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

::: lib/pk11wrap/pk11akey.c
@@ +1999,5 @@
>  	    return newKey;
>      }
>      destSlot = privKey->pkcs11Slot;
> +    if (PK11_Authenticate(destSlot, PR_TRUE, privKey->wincx) != SECSuccess) {
> +	return NULL;

PORT_SetError()?

@@ +2031,5 @@
>  
>      PK11_SETATTRS(attrs, CKA_TOKEN, &cktrue, sizeof(cktrue)); attrs++;
>  
> +    if (PK11_Authenticate(slot, PR_TRUE, wincx) != SECSuccess) {
> +    	return NULL;

PORT_SetError()

::: lib/pk11wrap/pk11merge.c
@@ +1129,5 @@
> +	    if (PK11_GetAttributes(arena, targetSlot, targetTrustID,
> +		    &targetTemplate, 1) != CKR_OK) {
> +		    rv = SECFailure;
> +		    goto done;
> +        }

Failures below in pk11_SetAttributes don't cause you to exit the loop, so why does this?

::: lib/pk11wrap/pk11skey.c
@@ +1087,5 @@
>      /* Get session and perform locking */
>      if (isToken) {
> +        if (PK11_Authenticate(symKey->slot,PR_TRUE,wincx) != SECSuccess) {
> +            PK11_FreeSymKey(symKey);
> +            return NULL;

PORT_SetError()

@@ +1144,5 @@
>  
>      PK11_SETATTRS(attrs, CKA_TOKEN, &cktrue, sizeof(cktrue)); attrs++;
>  
> +    if (PK11_Authenticate(slot, PR_TRUE, wincx) != SECSuccess) {
> +	return NULL;

PORT_SetError()

Comment 3

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #2)
> > +    if (PK11_Authenticate(destSlot, PR_TRUE, privKey->wincx) != SECSuccess) {
> > +	return NULL;
> 
> PORT_SetError()?

PK11_Authenticate sets errors already, should I set another error anyway if it fails?

> ::: lib/pk11wrap/pk11merge.c
> @@ +1129,5 @@
> > +	    if (PK11_GetAttributes(arena, targetSlot, targetTrustID,
> > +		    &targetTemplate, 1) != CKR_OK) {
> > +		    rv = SECFailure;
> > +		    goto done;
> > +        }
> 
> Failures below in pk11_SetAttributes don't cause you to exit the loop, so
> why does this?

well, the problem here is whether we should continue the loop if something goes wrong or not. In order not to change behaviour we should probably not exit the loop. But then checking the return value is useless as it gets overriden (we'd trade an unchecked return value for an unused value).

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: checking error values

I'm only setting an error return value in pk11merge now instead of returning. This should ensure that the behaviour doesn't change.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: checking error values

fixed indentation

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8717442 [details] [diff] [review]
checking error values

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

::: lib/pk11wrap/pk11merge.c
@@ +1124,5 @@
>  	    targetTemplate.ulValueLen = sourceTemplate.ulValueLen = 0;
> +	    CK_RV lrv = PK11_GetAttributes(arena, sourceSlot, id, &sourceTemplate, 1);
> +	    if (lrv != CKR_OK) {
> +		    rv = SECFailure;
> +		    error = PORT_GetError();

nit: please use four spaces for indentation here and below.

@@ +1135,3 @@
>  	    if (pk11_mergeTrustEntry(&targetTemplate, &sourceTemplate)) {
>  		/* source wins, write out the source attribute to the target */
> +		lrv = pk11_setAttributes(targetSlot, targetTrustID, 

pk11_setAttributes() returns a SECStatus, so we should probably just leave this line as-is. I wonder if the top-level variable should have a different name, Coverity might complain about shadowing.

@@ +1152,5 @@
> +	CK_RV lrv = PK11_GetAttributes(arena, sourceSlot, id, &sourceTemplate, 1);
> +	if (lrv != CKR_OK) {
> +	    rv = SECFailure;
> +	    error = PORT_GetError();
> +    }

Nit: formatting seems a little off here, and Coverity might complain about variable shadowing below?

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: check shift value

https://hg.mozilla.org/projects/nss/rev/825c88866c99

Comment 8

[Franziskus Kiefer [:franziskus]]

backed out due to bustage, e.g. [1]

https://hg.mozilla.org/projects/nss/rev/06054c817687


[1] https://bot.nss-crypto.org:8011/builders/1-cpp11-linux-x64-DBG/builds/220/steps/shell/logs/stdio

Comment 9

[Franziskus Kiefer [:franziskus]]

> merge.sh: Merge Tests ===============================
> merge.sh: Creating an SDR key & Encrypt
> sdrtest -d . -o /home/buildbot/slavedir/1-linux-x64-DBG/hg/tests_results/security/nss-vm-centos6-1.1/tests.v3.5324 -t Test2 -f ../tests.pw
> merge.sh: #3462: Creating SDR Key  - PASSED
> merge.sh: Merging in Key for Existing user
> certutil --merge --source-dir ../dave -d . -f ../tests.pw -@ ../tests.pw
> certutil: Could not merge object  (type Trust): Certificate extension not found.
> certutil: Could not merge object  (type Trust): Certificate extension not found.
> certutil: Could not merge object  (type Trust): Certificate extension not found.
> merge.sh: #3463: Merging Dave  - FAILED

Comment 10

[Robert Relyea]

Comment on attachment 8732206 [details] [diff] [review]
check shift value

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

Most of these are purposeful non-checks.

I'm also not so clear we should be checking the return code from PK11_Authenticate().

In general we prefer to fail in the actual attempted function that might need authentication rather than fail because the authenticate failed.

::: lib/pk11wrap/pk11merge.c
@@ +1130,5 @@
> +	    if (PK11_GetAttributes(arena, targetSlot, targetTrustID,
> +	                           &targetTemplate, 1) != CKR_OK) {
> +		rv = SECFailure;
> +		error = PORT_GetError();
> +	    }

This is wrong. Not reading the attribute should not be fatal. Not all tokens support all trust attributes. If the attribute read fails, The template length (ulValueLen) should be set to CK_UNAVAILABLE_INFORMATION, or left as '0'. pk11_mergeTrustEntry() will treat the result and CKT_NSS_TRUST_UNKNOWN. (see pk11_mergeTrustEntry). If we want to make that explicit, we can check the return code and set ulValueLen to '0' on failure.

@@ +1135,4 @@
>  	    if (pk11_mergeTrustEntry(&targetTemplate, &sourceTemplate)) {
>  		/* source wins, write out the source attribute to the target */
> +		if (pk11_setAttributes(targetSlot, targetTrustID,
> +			&sourceTemplate, 1) != SECSuccess) {

We shouldn't fail just because we couldn't write. pk11_merge is a best effort function and merges as 'best it can'.

@@ +1151,5 @@
> +	if (PK11_GetAttributes(arena, sourceSlot, id, &sourceTemplate, 1)
> +	    != CKR_OK) {
> +	    rv = SECFailure;
> +	    error = PORT_GetError();
> +    }

Like above, don't fail on the failure to read the attribute. Note the if statement below, which is checking the validity of the read attribute.

Comment 11

[Franziskus Kiefer [:franziskus]]

> This is wrong. Not reading the attribute should not be fatal.

This sounds like a major design flaw of the function to me then. If the function returns an error that could for example be an invalid session handle, we can't simply ignore it. Having the caller iterate over all possible error codes here in order to catch fatal errors doesn't sound like a sensible approach to me either. If CK_UNAVAILABLE_INFORMATION is the only "non-error" error that's returned we single that one out and fail in all other cases.

> If we want to make that explicit, we can check the return code and set ulValueLen to '0' on failure.

Or check for 0, that's fine with me as well. But we have to explicitly check the return value.

Comment 12

[Franziskus Kiefer [:franziskus]]

Attachment: pk11-return-values.patch

I#m setting |sourceTemplate.ulValueLen = 0| instead of failing when PK11_GetAttributes fails.
This seems to be working [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=51d43668dcb8585f38675a736f77c83575d2bc20&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded

Comment 13

[Robert Relyea]

Comment on attachment 8786387 [details] [diff] [review]
pk11-return-values.patch

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

r+ for the merge changes. 

Not so keen on the PK11_Authenticate checks.

bob

::: lib/pk11wrap/pk11akey.c
@@ +1999,5 @@
>      }
>      destSlot = privKey->pkcs11Slot;
> +    if (PK11_Authenticate(destSlot, PR_TRUE, privKey->wincx) != SECSuccess) {
> +	return NULL;
> +    }

So this is fine to a point, but it could cause some issues.

Say you are in a multi-threaded application (firefox or thunderbird) and you have two different threads pop up password dialogs (it can happen a lot in thunderbird). It's possible that I correctly type my password in one of those dialogs and hit ok, then dismiss the rest of the dialogs. Currently everything works fine, but with these checks I have to type my password in each of those dialogs.

In general, we purposefully ignore the return code from PK11_Authenticate(). If PK11_Authenticate fails, then the PKCS #11 module will fail in the next call with the error code "Unauthenticated". If it doesn't fail, then we really didn't need to authenticate anyway (and our guess that we needed to authenticate was wrong).

We should only check the error code for PK11_Authenticate() if we really do want to make sure we are logging into the token. Checks at the application level, or if we are going to cache a password or something. Deep in pk11wrap it's almost never the right thing to do.

@@ +2031,5 @@
>      PK11_SETATTRS(attrs, CKA_TOKEN, &cktrue, sizeof(cktrue)); attrs++;
>  
> +    if (PK11_Authenticate(slot, PR_TRUE, wincx) != SECSuccess) {
> +    	return NULL;
> +    }

Same issue.

::: lib/pk11wrap/pk11merge.c
@@ +1128,5 @@
> +	    }
> +	    if (PK11_GetAttributes(arena, targetSlot, targetTrustID,
> +	                           &targetTemplate, 1) != CKR_OK) {
> +		sourceTemplate.ulValueLen = 0;
> +	    }

These look good!

::: lib/pk11wrap/pk11skey.c
@@ +1088,5 @@
>      if (isToken) {
> +        if (PK11_Authenticate(symKey->slot,PR_TRUE,wincx) != SECSuccess) {
> +            PK11_FreeSymKey(symKey);
> +            return NULL;
> +        }

Same issue.