Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NSS rc_crypto replacement part 1 #998

Merged
merged 11 commits into from Apr 18, 2019

Conversation

Projects
None yet
3 participants
@eoger
Copy link
Contributor

commented Apr 16, 2019

Connects to #955.

Implemented crypto primitives

  • HMAC-SHA-256
  • HKDF-SHA-256
  • RNG
@rfk
Copy link
Member

left a comment

It wasn't clear whether this was ready for review but I wanted to leave some preliminary comments.

Sticking with the ring API seems to be working out nicely! My one suggestion would be to try to optimize this code for auditability, which means comments and docs to help guide the reader through what each file is doing. For example:

  • A doc comment on each of the structs to briefly explain what they're doing, particularly the ones that are compile-time wrappers designed to ensure correct behaviour. This will help a future reviewer who may be familiar with NSS but not with the ring API.
  • Brief inline comments to explain the major uses of the NSS API, explaining what we expect each step to accomplish. This will help a future reviewer check that what we're doing is what we actually intended.
Show resolved Hide resolved components/fxa-client/src/http_client/browser_id.rs Outdated
Show resolved Hide resolved components/fxa-client/src/scoped_keys.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/constant_time.rs

pub fn extract_and_expand(
salt: &hmac::SigningKey,
secret: &[u8],

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

Huh. It seems weird to me that salt is a SigningKey while secret is raw bytes, but this matches what ring does so shrug...I guess it just need to be this way in order for the underlying hmac calls to work correctly?

This comment has been minimized.

Copy link
@eoger

eoger Apr 17, 2019

Author Contributor

Correct, from ring doc:

Salts have type hmac::SigningKey instead of &[u8] because they are frequently used for multiple HKDF operations, and it is more efficient to construct the SigningKey once and reuse it. Given a digest algorithm digest_alg and a salt salt: &[u8], the SigningKey should be constructed as hmac::SigningKey::new(digest_alg, salt).

let info = hex::decode("f0f1f2f3f4f5f6f7f8f9").unwrap();
let expected_out = hex::decode(
"3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf34007208d5b887185865",
)

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

👍 I independently checked these test vectors against a known-good implementation

Show resolved Hide resolved components/support/rc_crypto/src/hmac.rs
Show resolved Hide resolved components/support/rc_crypto/src/p11.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/rand.rs
Ok(out)
fn hmac(&self, ciphertext: &[u8]) -> Result<Signature> {
let key = SigningKey::new(&digest::SHA256, self.hmac_key());
Ok(hmac::sign(&key, ciphertext)?)

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

Excellent cleanup here.

As discussed in meeting, a future iteration on this code should aim to move the whole "check hmac and decrypt" machinery into a single aead::open call of some sort. It will be complicated by sync needing to handle the ciphertext and hmac as two separate fields in the JSON, but I think it would be fine to require this code to concatenate them into a single buffer before calling rc_crypto to decrypt.

@@ -152,7 +136,7 @@ impl KeyBundle {
/// and the generated iv.
pub fn encrypt_bytes_rand_iv(&self, cleartext_bytes: &[u8]) -> Result<(Vec<u8>, [u8; 16])> {
let mut iv = [0u8; 16];
openssl::rand::rand_bytes(&mut iv)?;
rand::fill(&mut iv)?;

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

Having this use a consistent RNG with the rest of the code is also a nice win.

Show resolved Hide resolved components/push/src/error.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/constant_time.rs
Show resolved Hide resolved components/support/rc_crypto/src/p11.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/p11.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/constant_time.rs

@eoger eoger force-pushed the nss-rc-crypto-replacement branch 3 times, most recently from 1b769ab to 57c7ba5 Apr 17, 2019

@eoger eoger requested review from rfk and thomcc and removed request for rfk Apr 17, 2019

@eoger eoger changed the title Nss rc crypto replacement NSS rc_crypto replacement part 1 Apr 17, 2019

@eoger eoger force-pushed the nss-rc-crypto-replacement branch from 3125320 to fa09ab8 Apr 17, 2019

@rfk

rfk approved these changes Apr 18, 2019

Copy link
Member

left a comment

I'm happy with the shape of this from a "the crypto bits look right to me" perspective; @thomcc could you please sign off on the macros and mechanics of the C bindings since I don't feel qualified to check for any oversights there.

}
ensure_nss_initialized();
let result = unsafe {
nss_sys::NSS_SecureMemcmp(

This comment has been minimized.

Copy link
@rfk

rfk Apr 18, 2019

Member

👍 great!

Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/lib.rs
if base16::decode_slice(expected_hmac, &mut decoded_hmac).is_err() {
log::warn!("Garbage HMAC verification string: contained non base16 characters");
return Ok(false);

This comment has been minimized.

Copy link
@rfk

rfk Apr 18, 2019

Member

I am very, very pleased to see this throwing an error rather than returning OK(false), excellent cleanup!

@thomcc
Copy link
Contributor

left a comment

I'd probably like to give this another pass after these fixes are made. None are large but there are several.

Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs Outdated
ulInfoLen: info.len() as u64 as CK_ULONG,
});
let ptr_hkdf_params = Box::into_raw(hkdf_params);
let mut params = SECItem {

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 18, 2019

Contributor

This api is grody as hell.

Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs
Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/digest.rs Outdated
Show resolved Hide resolved components/support/rc_crypto/src/digest.rs Outdated

// Remap some constants.
pub const SECSuccess: SECStatus = _SECStatus_SECSuccess;
pub const SECFailure: SECStatus = _SECStatus_SECFailure;
pub const PR_FALSE: PRBool = 0;
pub const PR_TRUE: PRBool = 1;
pub const CK_FALSE: CK_BBOOL = 0;

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 18, 2019

Contributor

Gross.

Show resolved Hide resolved components/fxa-client/src/http_client/browser_id.rs Outdated
Show resolved Hide resolved components/fxa-client/src/http_client/browser_id.rs Outdated

@eoger eoger force-pushed the nss-rc-crypto-replacement branch from f999fc7 to 63eacd1 Apr 18, 2019

@eoger eoger requested a review from thomcc Apr 18, 2019

@thomcc

thomcc approved these changes Apr 18, 2019

Copy link
Contributor

left a comment

Well, hopefully this is all fine. I feel like there's a lot of potential cleanup we could do around SECItem in particular, but for now... It's fine.

One last thing though, could you go through and make sure all our integer casts (e.g. blah as <some integer>) are either checked or infallible? You can do a checked cast with try_from. If we get this wrong it can easily lead to out of bounds reads/writes which would be quite bad.

) -> Result<SymKey> {
let mut item = SECItem {
type_: SECItemType::siBuffer,
data: buf.as_ptr() as *mut c_uchar,

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 18, 2019

Contributor

This kind of sucks that we have to cast *const u8 to *mut u8 here, but it shouldnt' write to it... hopefully.

Show resolved Hide resolved components/support/rc_crypto/src/rand.rs
Show resolved Hide resolved components/support/rc_crypto/src/hkdf.rs Outdated
unsafe fn PK11_FreeSymKey(symKey: *mut PK11SymKey);
unsafe fn PK11_DestroyContext(context: *mut PK11Context, freeit: PRBool);
unsafe fn PK11_GetInternalSlot() -> *mut PK11SlotInfo;
unsafe fn PK11_ImportSymKey(slot: *mut PK11SlotInfo, r#type: CK_MECHANISM_TYPE, origin: PK11Origin::Type, operation: CK_ATTRIBUTE_TYPE, key: *mut SECItem, wincx: *mut c_void) -> *mut PK11SymKey;

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 18, 2019

Contributor

We could say key: *const SECItem to make things closer to how they are in reality, but it's probably better to keep this mirroring the real declaration, even if it's not const-correct.

Show resolved Hide resolved components/support/rc_crypto/src/rand.rs Outdated

eoger added some commits Apr 16, 2019

@eoger eoger force-pushed the nss-rc-crypto-replacement branch from 63eacd1 to 37f8482 Apr 18, 2019

@eoger eoger merged commit 79764fb into master Apr 18, 2019

10 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
ci/circleci: Carthage build Your tests passed on CircleCI!
Details
ci/circleci: Check Rust dependencies Your tests passed on CircleCI!
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Deploy website Your tests passed on CircleCI!
Details
ci/circleci: Lint Rust with clippy Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@eoger eoger deleted the nss-rc-crypto-replacement branch Apr 18, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Thanks for the thorough reviews @thomcc @rfk !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.