NSC_GetAttributeValue returns CKR_GENERAL_ERROR instead of CKR_ATTRIBUTE_TYPE_INVALID using the new sqlite3 backend

RESOLVED FIXED in 3.12.8

Status

NSS
Libraries
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Alex Dupre, Assigned: Robert Relyea)

Tracking

(Blocks: 1 bug)

3.12.4
3.12.8

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIPS)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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
Alex, thanks for your analysis.
Would you care to contribute a patch?
Assignee: nobody → rrelyea
(Reporter)

Comment 2

8 years ago
Created attachment 410182 [details] [diff] [review]
Patch

Anything else? :-P
Comment on attachment 410182 [details] [diff] [review]
Patch

Thanks.  I'll ask Bob to review this sqlite3 code
Attachment #410182 - Flags: review?(rrelyea)
(Assignee)

Comment 4

8 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

8 years ago
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.
(Reporter)

Comment 6

8 years ago
Created attachment 410446 [details] [diff] [review]
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).
(Assignee)

Comment 7

8 years ago
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
Attachment #410446 - Flags: review+
(Assignee)

Comment 8

8 years ago
Comment on attachment 410182 [details] [diff] [review]
Patch


the second patch was superior.

bob
Attachment #410182 - Flags: review?(rrelyea) → review-
Summary: C_GetAttributeValue returns CKR_GENERAL_ERROR instead of CKR_ATTRIBUTE_TYPE_INVALID using the new sqlite3 backend → NSC_GetAttributeValue returns CKR_GENERAL_ERROR instead of CKR_ATTRIBUTE_TYPE_INVALID using the new sqlite3 backend
This bug concerns code inside the FIPS boundary.
Whiteboard: FIPS

Updated

8 years ago
Target Milestone: --- → 3.13
(Reporter)

Comment 10

8 years ago
Created attachment 443078 [details] [diff] [review]
Working patch for second approach

The same patch, but tested and working.
Attachment #410446 - Attachment is obsolete: true
Blocks: 544289
Version: unspecified → 3.12.4
Created attachment 443123 [details] [diff] [review]
Alex's "working patch for second approach" as CVS diff

This converted patch should be easier to review.
Attachment #410182 - Attachment is obsolete: true
Attachment #443078 - Attachment is obsolete: true
Attachment #443123 - Flags: review?(rrelyea)
(Assignee)

Comment 12

8 years ago
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
Attachment #443123 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Bob, YOU're planning to commit the patch you r+'ed.  Right?
(Assignee)

Comment 14

8 years ago
yes.
(Assignee)

Comment 15

8 years ago
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

7 years ago
Created attachment 458854 [details] [diff] [review]
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.
Attachment #458854 - Flags: review?(rrelyea)

Comment 17

7 years ago
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
Attachment #458854 - Attachment description: Fix memory leak of newStr → Fix memory leak of newStr (checked in)

Comment 18

7 years ago
Created attachment 458890 [details] [diff] [review]
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.
Attachment #458890 - Flags: review?(rrelyea)
(Assignee)

Comment 19

7 years ago
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.
Attachment #458854 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 20

7 years ago
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..
Attachment #458890 - Flags: review?(rrelyea) → review+

Comment 21

7 years ago
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
Attachment #458890 - Attachment description: Don't need to free newStr at the end of sdb_GetAttributeValueNoLock → Don't need to free newStr at the end of sdb_GetAttributeValueNoLock (checked in)

Comment 22

7 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: 3.13 → 3.12.8
You need to log in before you can comment on or make changes to this bug.