391291 - Shared Database Integrity checks not yet implemented.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

The shared database design calls for integrity checks on certain critical attributes. Those checks are currently not implemented in the NSS 3.12 Alpha.

They need to be implemented before Shared Databases are turned on by default in applications as they affect database compatibility.

Comment 1

[Robert Relyea]

Attachment: Integrity patch

This patch implements integrity checking for certain non-encrypt attributes that are critical to our system integrity.

----- General information -----------

Those attributes are:
   1) Trust object hashes and trust values.
   2) public key values.

Certs themselves are considered properly authenticated by virtue of their signature, or their matching hash with the trust object.

Integrity is only check for objects coming from shared databases. Older dbm style databases have no integrity checks. Integrity is also only checked when the token is logged in, as Integrity is guarrenteed by the master password.

Tokens which have no key database (and therefore no master password) do not have any stored integrity values. Integrity values are stored in the key database, since the integrity data is tightly coupled to the key database password.

-------------------------- more detail on integrity implementation -----------

Integrity values are stored in a special table in the key database called the 'metadata'. This table could store other types of meta data as well in the future. Currently the password check is also stored in the metadata table.

Integrity entries are indexed by the string sig_[cert/key]_{ObjectID}_{Attribute}

Where cert/key indicates which logical database the actual object is stored in, {ObjectID} is the id of the object and {Attribute} is the attribute this integrity entry is for.

Entries are check when the user tries to get and attribute. Once the attributes are fetched, GetAttributes loops through the attributes looking for attributes that require integrity checks. When such an attribute is found, the integrity check entry is fetched and an HMAC is calculated for the attribute. If the attribute matches the integrity entry, processing proceeds. If it doesn't that attribute data is cleared and CKR_SIGNATURE_INVALID is returned.

Integrity checks are PBMAC1 data structures defined in pkcs5. Since pkcs5 v1 had not integrity checks and pkcs12 has not definition for storing purely mac data,the shared DB integrity checks uses pkcs5 v2 to store that pbe and mac data. This patch turns on pkcs5 v2 as a result. The PBMAC1 is using SHA256 for a MAC operation. The hmac data is formed by taking an hmac over the objectId, attribute type, and data value of the attribute.

------------------ other semantics -------------------------

Because of this change, it is not possible to update certain values in the database without a password. Public keys should not be a problem since they are almost always created or imported at the same time as their private counterparts (which always required a password to update). Trust objects, on the other hand, are often updated without a password. This patch includes a patch to certutil which retries trust update operations if they fail because the user was not logged in.This also required updating some of the stan code to properly return error codes which could be mapped to the appropriate NSS error codes.

Comment 2

[Robert Relyea]

Other reviews or even partial review comments welcome! on this patch.

Comment 3

[Kai Engert [:KaiE:]]

I have started to look at this.

I think you have written a very helpful piece of text in this bug, I propose to put these words directly into the source files. When future maintainers look at the code, they might not find the link to this bug report and the text.

If you're worried the documentation and sources might get out of sync over time, you could clearly indicate that documentation with "general notes on integrity testing, last updated August 2007" - this would indicate readers the comment is not necessarily up to date, but still might be very helpful when reading the code.


If there is an easy way to, you could avoid having the string "sig_%s_%08x_%08x" twice in the code.


(In reply to comment #0)
> The shared database design calls for integrity checks on certain critical
> attributes. Those checks are currently not implemented in the NSS 3.12 Alpha.
> 
> They need to be implemented before Shared Databases are turned on by default in
> applications as they affect database compatibility.

I absolutely agree integrity checks are a good thing, I'm just not sure I fully understand your motivation for introducing them. Could you give an example of a scenario where things will go wrong without integrity checks?

I'm guessing: Are you worried that app v2 introduces new attributes ax, app v1 doesn't know about it, app v1 tries to modify the object and fails to update attribute ax. When app v2 tries to access the object, it will discover that attribute ax is not in sync and will automatically erase it?

Comment 4

[Kai Engert [:KaiE:]]

Unless I mention something as "must fix", I'm only giving food for thought, feel free to address or skip.

In CERT_MapStanError, renaming error1 to prev_error would improve readability, I think.

if (errorStack != NULL) and (errorStack[0] == 0)
  then error1/prev_error will remain uninitialized,
(not a problem with your current code)

I wonder if STAN_MAP_ERROR would produce better code if you used "switch" and your macro used "case" (instead of the series of: if else if else ...)
If you're interested to simplify this code, I see no need to explicitly use a return statement for each scenario, you could simply assign to a variable, and use a single PORT_SetError at the end of the function.


In file stanpcertdb.c you have a section where reference the const NSS error values that are defined elsewhere, e.g.:
  const NSSError NSS_ERROR_NO_ERROR;
  const NSSError NSS_ERROR_INTERNAL_ERROR;
  ...
I think you want to declare them as "extern"?
(you do so in other places)


I have not reviewed the pkcs#5 template definitions.
Please let me know if I should read up in detail to be able to review.


In do_xor you're switching to always use the code formerly called "paranoia", so I guess it's ok to do.
But you're adding other code #ifdef'ed by USE_UINTS, I can't find this define anywhere in NSPR or NSS.

(hitting commit to not lose what I've written so far... more comments coming up)

Comment 5

[Kai Engert [:KaiE:]]

Are you sure nsspkcs5_PBKFD2_F and nsspkcs5_PBKDF2 (which you enabled) will never be called with NULL pointer params/items? (the functions don't check before derefencing)

In sec_pkcs5_des you call CBC_PadBuffer with a value of 8, what's the meaning? Is there a self-explanatory constant definition you could use? Maybe DES_KEY_LENGTH? Same for sec_pkcs5_aes where you're using values 16 and 64.


### Probably a bug?
In nsc_pbe_key_gen you introduce pbkd2_params and init it, but you never use it. You always use the old pbe_params.

Comment 6

[Kai Engert [:KaiE:]]

>     /* only Key databases have password entries */

You might want to update the comment to say "have metadata entries" (or "such" entries...)


What's the meaning of the string "password" that you have repeatedly throughout the sources? Is it the password used to access the sqlite database? Could you turn this into a string constant, like NSS_SQLITE_DB_PW ?


This is a big patch. For the second half, so far I only skimmed through it.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275713 [details] [diff] [review]
Integrity patch

Review comments by source file:

1. stanpcertdb.c  
Unless this file has previously included some header file that already
declared all those NSS_ERROR_ values as extern, these declarations
will be definitions, causing all those variables to be defined here
(for this source file) with the value zero.  

Required: put the word "extern" in front of all these.  

2. I appreciate you implementing CERT_MapStanError().
I think this actually may resolve another NSS RFE.
It needs to be declared in an NSS private header file.
There are lots of other places that need to call it, too.
This is a MUST.

The necessity of implementing it as a big long if-elseif-elseif-elseif
chain shows the primary weakness of the approach used in stan.
It's not possible to have a switch statement whose cases are the 
error symbol names when those names are the names of global variables.
I heartily recommend creating an enumerated type, and making that 
available so that this function can be implemented as a switch.
I think there would be many other benefits, too.  
Not a MUST for this patch, but maybe we should have a separate bug 
for this.

3. The patch adds some SHA224 symbols to softoken/pkcs11t.h, mostly
CKM_SHA224_* symbols.  And it adds references to those CKM_SHA224 
symbols in the secoid table.  But there is no other use of them,
and we don't have an implementation of SHA224 anywhere in NSS.
So I wonder, was the patch to softoken/pkcs11t.h intended to be 
part of a patch for another bug/RFE?  Is there a SHA224 implementation
somewhere waiting to be reviewed?

4. In softoken/sdb.c, you've created a new "metaData" table to replace 
the short-lived "password" table.  Since the "password" table will 
never live in any real NSS 3.12 release, let's plan to remove all 
vestiges of the password table before we release NSS 3.12.
Probably should be a separate bug for that.

5. There are lots of strings containing SQL commands in sdb.c.
The way they are defined, they will be in the shared lib's data 
segment.  Let's put them into the text segment (making them read-
only) by declaring them as initializers of static arrays of const 
char rather than as argument strings, or initializers of pointers.

6. In sftkdb.c, please rename the following functions by appending 
the word "Attribute" to their existing names.  Also, let's maintain
the NSS coding style of capitalizing the first character after the 
first underscore.

sftkdb_decrypt
sftkdb_dencrypt
sftkdb_sign
sftkdb_verify
sftkdb_isULONG
sftkdb_isPrivate
sftkdb_isAuthenticated
(and maybe others)

These two new functions need a noun to identify the kind of thing
for which the signature is being gotten or put.  
sftkdb_getSignature
sftkdb_putSignature

Maybe sftkdb_GetObjectSignature and sftkdb_PutObjectSignature ?
Call these name changes a MUST.

7. One overall comment.  There are a lot of functions in the 
shared DB code (and in the new legacy code) that don't have any
block comments explaining what they are intended to do.  Future 
maintainers of this code will have to figure that out for themselves.
So, please put a block comment at the beginning of each new function
explaining what it is supposed to do, in enough detail that someone
can use that comment as a specification, to see if the function does
as intended, or not.  Obviously, this is not a MUST for every 
function at this time.

r+ with the above MUSTs done, and Kai's review MUSTs.

Comment 8

[Robert Relyea]

Patch checked in:

Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.116; previous revision: 1.115
done
Checking in lib/base/errorval.c;
/cvsroot/mozilla/security/nss/lib/base/errorval.c,v  <--  errorval.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/certdb/certi.h;
/cvsroot/mozilla/security/nss/lib/certdb/certi.h,v  <--  certi.h
new revision: 1.18; previous revision: 1.17
done
Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.76; previous revision: 1.75
done
Checking in lib/dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.41; previous revision: 1.40
done
Checking in lib/softoken/lowpbe.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowpbe.c,v  <--  lowpbe.c
new revision: 1.18; previous revision: 1.17
done
Checking in lib/softoken/lowpbe.h;
/cvsroot/mozilla/security/nss/lib/softoken/lowpbe.h,v  <--  lowpbe.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/softoken/padbuf.c;
/cvsroot/mozilla/security/nss/lib/softoken/padbuf.c,v  <--  padbuf.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.95; previous revision: 1.94
done
Checking in lib/softoken/pkcs11t.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11t.h,v  <--  pkcs11t.h
new revision: 1.18; previous revision: 1.17
done
Checking in lib/softoken/sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/sdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.h,v  <--  sdb.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/sftkdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v  <--  sftkdb.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/softoken/sftkdbt.h;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdbt.h,v  <--  sftkdbt.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.16; previous revision: 1.15
done
Checking in lib/softoken/legacydb/keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/softoken/legacydb/lgdb.h;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgdb.h,v  <--  lgdb.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/softoken/legacydb/lginit.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lginit.c,v  <--  lginit.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/softoken/legacydb/lowkeyti.h;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkeyti.h,v  <--  lowkeyti.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/util/secalgid.c;
/cvsroot/mozilla/security/nss/lib/util/secalgid.c,v  <--  secalgid.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.38; previous revision: 1.37
done
Checking in lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.24; previous revision: 1.23
done
Checking in tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.44; previous revision: 1.43
done

Comment 9

[Robert Relyea]

Attachment: Patch as checked in (with review comments addressed).

Comment 10

[Robert Relyea]

Issues not directly addressed in the code:

Kaie comment 5 "Null Pointers checks for nsspkcs5_PBKD2_F/nsspkcs5_PBKDF2".

Those are both static private functions which cannot take NULL pointers for  params/items.


Nelson comment 7 (2, second paragraph) - also Kaie comment 4 3rd paragraph:
"RE: switch statement and CERT_MapStanErrors()"

I've openned bug 391597. Nelson correctly surmised why I didn't use a switch statement. I agree that enums would be preferable.

Nelson comment 7 (3): SHA_224.

SHA_224 has shown up in a few standards. PKCS 5 v2.1 defined the HMAC OIDS for SHA_224, so I added them along with the HMAC OIDS for the other hashes. PKCS #11 added SHA 224 Mechanisms in 2.20 amendment 3 (same amendment that added CAMELLIA). Nelson is correct, we do not have an implementation, nor is there an RFE bug openned to do such an implementation. SHA_224 is defined in:

http://www.ipa.go.jp/security/rfc/RFC3874EN.html