526231 - NSC_GetAttributeValue returns CKR_GENERAL_ERROR instead of CKR_ATTRIBUTE_TYPE_INVALID using the new sqlite3 backend

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Alex Dupre]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; it-IT; rv:1.8.1.20) Gecko/20090615 Firefox/2.0.0.20
Build Identifier: 3.12.4

Calling C_GetAttributeValue with the CKA_ALLOWED_MECHANISMS attribute type, returns CKR_GENERAL_ERROR, if the softoken backend is sqlite3. Using the legacydb the correct CKR_ATTRIBUTE_TYPE_INVALID is returned.
The reason is CKA_ALLOWED_MECHANISMS is a not supported attribute type, so the corresponding sqlite column doesn't exist; when nss tries to prepare the SELECT statement, it fails and map the sql error into CKR_GENERAL_ERROR.

Reproducible: Always

Steps to Reproduce:
1. Call C_GetAttributeValue with an unsupported attribute type (and using sqlite3 backend)

Actual Results:  
CKR_GENERAL_ERROR

Expected Results:  
CKR_ATTRIBUTE_TYPE_INVALID

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Alex, thanks for your analysis.
Would you care to contribute a patch?

Comment 2

[Alex Dupre]

Attachment: Patch

Anything else? :-P

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 410182 [details] [diff] [review]
Patch

Thanks.  I'll ask Bob to review this sqlite3 code

Comment 4

[Robert Relyea]

This is clearly a valid bug. I'm wondering why we aren't getting the correct error code, and maybe we are just mapping the wrong error code from sqlite3. I'm not excited about the explicit array check. First, if we increase the understood mechanisms in the future, but read an old database, we should get the ATTRIBUTE_TYPE_INVALID error for that object rather than GENERAL_ERROR.

I wonder if I'm mapping the wrong error code.

(also, if we are explicitly checking the attribute, we might as well just return the error when we find it doesn't match, rather then going through the query).

bob

Comment 5

[Alex Dupre]

It's difficult to map the sqlite error, because:
a) a wrong query returns a generic SQLITE_ERROR
b) one invalid attribute should not prevent getting other attributes

I agree that the array check is not the perfect solution, but it's the simplest and quite working. Maybe the known attribute list should be calculated at run-time from the actual db schema. I'm not going to create such patch ;-)

Your last comment doesn't apply because of (b)

From PKCS#11 specs:

Note that the error codes CKR_ATTRIBUTE_SENSITIVE, CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote true errors for C_GetAttributeValue. If a call to C_GetAttributeValue returns any of these three values, then the call must nonetheless have processed every attribute in the template supplied to C_GetAttributeValue. Each attribute in the template whose value can be returned by the call to C_GetAttributeValue will be returned by the call to C_GetAttributeValue.

Comment 6

[Alex Dupre]

Attachment: Another approach

This is another approach: instead of creating a sql query with all attributes, it creates a query for each attribute. This simplifies the sql error mapping.

I haven't re-indented the file to keep the patch small. I've not tested the patch, yet (it compiles, though).

Comment 7

[Robert Relyea]

Comment on attachment 410446 [details] [diff] [review]
Another approach

r+ for the SOFTOKEN_3_13 branch.

I'd like to see what the performance hit this has, but it's definitely more correct. I would probably eventually want to rework the patch to still try the entire search. If we get an SQLITE_ERROR, then try the attributes one at a time...

bob

Comment 8

[Robert Relyea]

Comment on attachment 410182 [details] [diff] [review]
Patch


the second patch was superior.

bob

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

This bug concerns code inside the FIPS boundary.

Comment 10

[Alex Dupre]

Attachment: Working patch for second approach

The same patch, but tested and working.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alex's "working patch for second approach" as CVS diff

This converted patch should be easier to review.

Comment 12

[Robert Relyea]

Comment on attachment 443123 [details] [diff] [review]
Alex's "working patch for second approach" as CVS diff

r+

This patch did not adjust the indent for the extended for loop. This was good for helping review, Checked-in version should have the proper indent.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Bob, YOU're planning to commit the patch you r+'ed.  Right?

Comment 14

[Robert Relyea]

yes.

Comment 15

[Robert Relyea]

Checked into the branch.

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.16.4.1; previous revision: 1.16
done


I see the bug is marked FIP2010 and I think the patch is has little FIPS effects, so I'll leave this open until I check it into the tip as well.

Comment 16

[Wan-Teh Chang]

Attachment: Fix memory leak of newStr (checked in)

Alex's patch (attachment 443123 [details] [diff] [review]) introduced a memory leak
(see bug 580137 comment 3).  This patch fixes the leak.

Comment 17

[Wan-Teh Chang]

Comment on attachment 458854 [details] [diff] [review]
Fix memory leak of newStr (checked in)

I checked in this patch on the NSS trunk (NSS 3.13)
before getting a review, because our Tinderboxes take
a very long time to cycle through and I need to check
in some other patches soon (so I need the Tinderboxes
to be green).

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.18; previous revision: 1.17
done

Comment 18

[Wan-Teh Chang]

Attachment: Don't need to free newStr at the end of sdb_GetAttributeValueNoLock (checked in)

Since we free newStr immediately after use (with no "goto loser"
statements in between), it's not necessary to free newStr at the
end of the function.  I won't push hard for this patch because
the original code is more defensive, but I found some other
functions in sdb.c that free newStr immediately after use and
don't free it at the end of function.

Comment 19

[Robert Relyea]

Comment on attachment 458854 [details] [diff] [review]
Fix memory leak of newStr (checked in)

r+ Yeah, when we added the loop, we could no longer depend on the full free op
at the end.

Comment 20

[Robert Relyea]

Comment on attachment 458890 [details] [diff] [review]
Don't need to free newStr at the end of sdb_GetAttributeValueNoLock (checked in)

r+ though leaving this is innocuous..

Comment 21

[Wan-Teh Chang]

Comment on attachment 458890 [details] [diff] [review]
Don't need to free newStr at the end of sdb_GetAttributeValueNoLock (checked in)

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

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.19; previous revision: 1.18
done

Comment 22

[Wan-Teh Chang]

Based on Bob's Comment 15, I checked in all three patches on the
NSS_3_12_BRANCH (NSS 3.12.8).  (When Bob wrote comment 15,
NSS_3_12_BRANCH had not been created, and the tip was version
3.12.x.)

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.16.6.1; previous revision: 1.16
done