Open Bug 516104 Opened 11 years ago Updated 10 years ago

Softoken erroneously returns a RO session when we ask for a RW session.

Categories

(NSS :: Libraries, defect, P2)

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: wtc, Assigned: rrelyea)

Details

Attachments

(2 files)

PK11_MapError maps CKR_SESSION_READ_ONLY to SEC_ERROR_LIBRARY_FAILURE:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11err.c&rev=1.10&mark=111,140#111

We are now using SEC_ERROR_LIBRARY_FAILURE to mean an NSS internal error,
But CKR_SESSION_READ_ONLY is not caused by an NSS internal error.
For example, if I try to import a certificate into a read-only token,
the token import returns CKR_SESSION_READ_ONLY.  Fortunately, PK11_ImportCert
overrides it with SEC_ERROR_ADDING_CERT:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11cert.c&rev=1.170&mark=911,921,927,937-938,941#910

But setting the error code to SEC_ERROR_LIBRARY_FAILURE in the
interim confused me when stepping through the code in the debugger.

We can use either the existing error code SEC_ERROR_READ_ONLY or
a new SEC_ERROR_READ_ONLY_TOKEN or SEC_ERROR_READ_ONLY_SESSION
error code.  Since CKR_ATTRIBUTE_READ_ONLY and
CKR_TOKEN_WRITE_PROTECTED are already mapped to SEC_ERROR_READ_ONLY,
I think we should just use SEC_ERROR_READ_ONLY.  The only issue is
that SEC_ERROR_READ_ONLY is documented to mean "Security library:
read-only database", which seems too narrow.
Attachment #400215 - Flags: review?(nelson)
> But CKR_SESSION_READ_ONLY is not caused by an NSS internal error.
> For example, if I try to import a certificate into a read-only token,
> the token import returns CKR_SESSION_READ_ONLY.

By definition, importing a certificate is done on a "session", and by 
definition, each and every session is either read/write or read-only, 
a condition that is established at the time that the session is established,
and remains unchanged for the life of the session.  

NSS's PK11wrap layer maintains information about sessions.  It knows whether
a session is read-only or read-write.  It has logic that detects when it 
needs to perform a read-write operation, but the current session is read-only
and it then attempts to switch to a read-write session.  

As I understand the design intent of the PK11wrap layer (which may be mistaken), it IS a logic error of the PK11wrap layer for it to attempt to 
import an object on a read-only session.  The fact that the PKCS#11 module 
had to remind the PK11wrap layer that it had attempted to do something 
illegal is prima-facie evidence of the pk11wrap layer's library failure.

If the token is a "read only" token, meaning that it is not possible to 
establish read-write sessions to that token, then the PK11wrap layer should
notice this, and should not attempt to import objects to that token.  
I agree that SEC_ERROR_READ_ONLY is the right error code, but that error condition should be detected by the PK11wrap layer BEFORE it even begins to  attempt to import an object into a read-only token.  The PK11wrap layer 
should report that error INSTEAD of attempting to do the illegal import.  

So, I would prefer to leave the error code mapping as is, and instead to find
the erroneous code in PK11wrap that failed to notice the read-only session,
and fix that code path to do the right thing.  

Bob, do you agree with my analysis?
If Bob agrees with my analysis, then I propose to change the summary of this
bug to 
"PK11wrap should not attempt to import a certificate on a read-only session"
It really depends on what is happening under the covers.

In general, the PK11wrap layer prefers to 'react' rather than 'anticipate'. Checking every possible reason for failure before trying an operation is not really efficient, can complicate the code, and has the potential of allowing an otherwise acceptable behavior.

That being said, creating a token object on a ro session should never work, and pk11wrap *does* create a r/w session to take this object on. What is happening is softoken is quitely changing the request to open a R/W session to a request create a R/O session.

>   if (slot->readOnly && (flags & CKF_RW_SESSION)) {
>        /* NETSCAPE_SLOT_ID is Read ONLY */
>        session->info.flags &= ~CKF_RW_SESSION;
>    }


Tracing the code, this looks like a bug in softoken. It appears softoken is accepting a request to create a RWsession even if the token is read only. So the long term solution is to fix softoken, but in the mean time, it's probably reasonable to change the mapping of CK_SESSION_READONLY to the same as CK_TOKEN_WRITE_PROTECTED (which is the error we should have gotten back from the C_OpenSession with Read/Write in softoken). In fact it's probably the right change anyway since softoken may not be the only PKCS #11 module with this bug.

bob
IMO, PK11wrap should never ask any PKCS#11 module to do something (anything)
that is, by definition of PKCS#11, wrong ("illegal").  It used to do a lot of
that, including such things as using zero as a session ID in requests to 
modules (zero is THE reserved invalid session ID, a.k.a CK_INVALID_SESSION).  

I cleaned a lot of that up in 3.12, and consequently, NSS works a lot better 
now with numerous third party PKCS#11 modules. IMO, PK11wrap likewise ought 
never to attempt to use a read-only session to perform operations that, by definition, require read-write sessions.  IMO, that is every bit as bad of a 
bug as attempting to use session ID zero.  

Now, IF it can be shown that there is some circumstance in which a module 
can legitimately and properly return CKR_SESSION_READ_ONLY to an operation 
that has been requested on a read-write session, then I will change my view
and agree to map CKR_SESSION_READ_ONLY to SEC_ERROR_READ_ONLY.  

Perhaps a compromise is to write some code that attempts to identify, after 
the fact, whether the session was R/O or R/W, and map CKR_SESSION_READ_ONLY 
to either SEC_ERROR_LIBRARY_FAILURE or SEC_ERROR_READ_ONLY depending on the
state of the session.
> IMO, PK11wrap should never ask any PKCS#11 module to do something (anything)
> that is, by definition of PKCS#11, wrong ("illegal").

That is exactly how the PK11wrap layer works... for everything. It isn't wrong to ask for something that's not allowed. It's wrong for the token to grant it.

The logic to determine if the complex set of events is legal for this particular token is correct. I will not muddy up the pk11wrap layer to try to detect every place where an operation *could* fail when the token should supply that information to me (the token knows perfectly well that it's R/O and RW sessions are illegal).

As a note: There is nothing 'illegal' about calling a token with an invalid Session of *any kind* (including CK_INVALID_SESSION). A token that does not return an error for an unknown session is already horribly broken. 

> PK11wrap likewise ought 
> never to attempt to use a read-only session to perform operations that

The only time it does is if the token mistakenly gives us a R-O session when we asked for a R/W session. As I said, this is an error in *SOFTOKEN*.

> Perhaps a compromise is to write some code that attempts to identify, after 
> the fact, whether the session was R/O or R/W, and map CKR_SESSION_READ_ONLY 
> to either SEC_ERROR_LIBRARY_FAILURE or SEC_ERROR_READ_ONLY depending on the
> state of the session.

This won't work because the softoken bug silently maps the session in question to a readonly session, so by the time the error occurs, it's indistinguishible.
Attachment #400215 - Flags: review?(nelson) → review+
Comment on attachment 400215 [details] [diff] [review]
Map CKR_SESSION_READ_ONLY to SEC_ERROR_READ_ONLY (checked in)

r+ rrelyea....

We should open a second bug against softoken and it it fixed in 3.13 as well.

bob
Bob, the issue is that third party PKCS#11 modules see that they're being
asked to do something they should NEVER be asked to do, such as use the
invalid session handle or do a write operation on a read-only session, and
they say "I'm dealing with a HORRIBLY BROKEN" application, and I won't 
tolerate this nonsense, so I will just start returning errors to ALL its 
requests."  That was the problem I solved when changed pk11wrap to 
absolutely stop ever asking modules to use CK_INVALID_SESSION.  

There's a big difference between trying to avoid ever doing an operation 
that *might* return an error (which I am not proposing) and trying to avoid
doing an operation that *MUST* *ALWAYS* return an error.  I'm saying we 
should avoid the latter. 

Now, perhaps you are saying that, in this instance that Wan-Teh encountered,
the fault is actually that the error code returned was erroneous, because
the session was NOT a read-only session.  In that case, I agree that that 
would be a softoken bug.  But again, it WOULD be a "library failure".
> paragraph 1....

1) PK11wrap has been designed to be reactive no proactive. Deal with it. I am not going to query the token for the billion possible states it could potentially be in, not am I going to build a state machine to try to predict what state a token is in... the token knows the state. It knows what it can and can't do. It should generate an error when it fails.
2) the CK_INVALID_SESSION issue was actually mostly bugs in the stan dev code, not pk11wrap... It's also a red herring.

Now I agree, we know we are trying to create a token object, we know we need to get a R/W session. NSS in fact does this. The bug is that the token is quietly returning a R/O session when we asked for a R/W session. The only way to know is to make another PKCS #11 call to determine whether the session really is R/O or R/W...all to detect a bug in the PKCS #11 module itself.

If the token functioned perfectly, then there would be no need to change the error code. Because softoken had this bug, and because it's possible other tokens followed it in this error, it seems reasonable to change the error we report. I'm willing to take that patch.... otherwise we can close this bug as INVALID (well actually change it to a softoken bug).

bob
The outcome of this bug is not what I expected.  I thought
it was yet another case of sloppy error code mapping.  Now
I realize that PK11_MapError maps some CKR_ error codes to
SEC_ERROR_LIBRARY_FAILURE intentionally.  That's worth a
comment in the source code because it wasn't obvious to me.
I've title to reflect the given issue.

RE: SEC_ERROR_LIBRARY_FAILURE - All instances of SEC_ERROR_LIBRARY_FAILURE are supposed to be causes where if we get it, it indicates that there is a bug in the library (either NSS or the token). Doing a quick review, that appears to be true of all cases except CKR_VENDOR_DEFINED (which really isn't an error code, per se, but a flag).

"Run home to momma" error (that is the error we return when no other error seems appropriate) has been SEC_ERROR_IO. Perhaps that should be a new error code SEC_ERROR_UNKNOWN_PKCS11_ERROR or SEC_ERROR_UNEXPECTED_PKCS11_ERROR). Currently at is returned by CKR_CANCEL, CKR_ATTRIBUTE_SENSITIVE (which really should have it's own error code), CKR_TOKEN_NOT_RECOGNIZED, and any error that is not explicitly in the list.

bob
Summary: PK11_MapError should not map CKR_SESSION_READ_ONLY to SEC_ERROR_LIBRARY_FAILURE → Softoken erroneously returns a RO session when we ask for a RW session.
Target Milestone: 3.12.5 → 3.12.7
Comment on attachment 400215 [details] [diff] [review]
Map CKR_SESSION_READ_ONLY to SEC_ERROR_READ_ONLY (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.7).

Checking in pk11err.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11err.c,v  <--  pk11err.c
new revision: 1.12; previous revision: 1.11
done
Attachment #400215 - Attachment description: Map CKR_SESSION_READ_ONLY to SEC_ERROR_READ_ONLY → Map CKR_SESSION_READ_ONLY to SEC_ERROR_READ_ONLY (checked in)
Add SEC_ERROR_UNKNOWN_PKCS11_ERROR, as Bob suggested in comment 10.
Explain why some PKCS11 errors are mapped to SEC_ERROR_LIBRARY_FAILURE.
Add error messages for the two errors added in bug 506966.
Attachment #452163 - Flags: review?(rrelyea)
Comment on attachment 452163 [details] [diff] [review]
Add SEC_ERROR_UNKNOWN_PKCS11_ERROR (checked in)

R+ including the password expired and password lock error codes.

bob
Attachment #452163 - Flags: review?(rrelyea) → review+
Comment on attachment 452163 [details] [diff] [review]
Add SEC_ERROR_UNKNOWN_PKCS11_ERROR (checked in)

I checked in this patch on the NSS trunk (NSS 3.13).

Checking in cmd/lib/SECerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.21; previous revision: 1.20
done
Checking in lib/pk11wrap/pk11err.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11err.c,v  <--  pk11err.c
new revision: 1.13; previous revision: 1.12
done
Checking in lib/util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.27; previous revision: 1.26
done

I added the error messages for SEC_ERROR_EXPIRED_PASSWORD and
SEC_ERROR_LOCKED_PASSWORD to NSS_3_12_BRANCH (NSS 3.12.8).

Checking in SECerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.20.2.1; previous revision: 1.20
done
Attachment #452163 - Attachment description: Add SEC_ERROR_UNKNOWN_PKCS11_ERROR → Add SEC_ERROR_UNKNOWN_PKCS11_ERROR (checked in)
I've fixed the error code mapping issues.

The remaining work is to fix the softoken bug that Bob described in comment 8.
Assignee: wtc → rrelyea
Target Milestone: 3.12.7 → 3.13
You need to log in before you can comment on or make changes to this bug.