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.

Sign up for GitHub

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

Constant time comparison for SigV4a #3174

Merged
merged 13 commits into from
Nov 16, 2023
Merged

Constant time comparison for SigV4a #3174

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0cc8d67
Constant time comparison for SigV4a
82marbag Nov 10, 2023
0555a90
Use const
82marbag Nov 10, 2023
3a22c95
Update Cargo.toml
82marbag Nov 13, 2023
6d7d645
Update init of BIG_N-2
82marbag Nov 13, 2023
6f19eca
Fix lint
82marbag Nov 13, 2023
dc15f07
Use static
82marbag Nov 13, 2023
359f169
Fix lint
82marbag Nov 13, 2023
0574c7f
expect() computation
82marbag Nov 14, 2023
294ed8b
Run rustfmt
82marbag Nov 14, 2023
86d9a5b
Specify the latest Crypto-Bigint version to ensure the latest version…
rcoh Nov 14, 2023
935f27f
Run CI manually
82marbag Nov 15, 2023
4c7cc68
Merge branch 'main' into constant-time-sigv4a
82marbag Nov 16, 2023
8f1e694
Pin subtle 2.5
82marbag Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws/rust-runtime/aws-sigv4/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ percent-encoding = { version = "2.1", optional = true }
regex = "1.5"
ring = { version = "0.17.5", optional = true }
sha2 = "0.10"
crypto-bigint = "0.5"
82marbag marked this conversation as resolved.
Show resolved Hide resolved
time = "0.3.5"
tracing = "0.1"
zeroize = { version = "^1", optional = true }
Expand Down
25 changes: 10 additions & 15 deletions aws/rust-runtime/aws-sigv4/src/sign/v4a.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,21 @@

use aws_smithy_runtime_api::client::identity::Identity;
use bytes::{BufMut, BytesMut};
use num_bigint::BigInt;
use once_cell::sync::Lazy;
use crypto_bigint::{U256, Encoding, CheckedAdd};
use p256::ecdsa::signature::Signer;
use p256::ecdsa::{Signature, SigningKey};
use std::io::Write;
use std::time::SystemTime;
use zeroize::Zeroizing;

const ALGORITHM: &[u8] = b"AWS4-ECDSA-P256-SHA256";
static BIG_N_MINUS_2: Lazy<BigInt> = Lazy::new(|| {
const BIG_N_MINUS_2: U256 = {
// The N value from section 3.2.1.3 of https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-186.pdf
// Used as the N value for the algorithm described in section A.2.2 of https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf
// *(Basically a prime number blessed by the NSA for use in p256)*
const ORDER: &[u32] = &[
0xFFFFFFFF, 0x00000000, 0xFFFFFFFF, 0xFFFFFFFF, 0xBCE6FAAD, 0xA7179E84, 0xF3B9CAC2,
0xFC632551,
];
let big_n = BigInt::from_slice(num_bigint::Sign::Plus, ORDER);
big_n - BigInt::from(2i32)
});
const ORDER: U256 = U256::from_be_hex("ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551");
ORDER.shr_vartime(1)
82marbag marked this conversation as resolved.
Show resolved Hide resolved
};

/// Calculates a Sigv4a signature
pub fn calculate_signature(signing_key: impl AsRef<[u8]>, string_to_sign: &[u8]) -> String {
Expand Down Expand Up @@ -64,14 +59,14 @@ pub fn generate_signing_key(access_key: &str, secret_access_key: &str) -> impl A
let tag = ring::hmac::sign(&key, &buf);
let tag = &tag.as_ref()[0..32];

let k0 = BigInt::from_bytes_be(num_bigint::Sign::Plus, tag);
let k0 = U256::from_be_bytes(tag.try_into().expect("convert to [u8; 32]"));

// It would be more secure for this to be a constant time comparison, but because this
// is for client usage, that's not as big a deal.
if k0 <= *BIG_N_MINUS_2 {
let pk = k0 + BigInt::from(1i32);
let d = Zeroizing::new(pk.to_bytes_be().1);
break SigningKey::from_bytes(&d).unwrap();
if k0 <= BIG_N_MINUS_2 {
let pk = k0.checked_add(&U256::ONE).unwrap_or(U256::ZERO);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it might be a change in behavior. What was the behavior for the + operator previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a change in behavior. k0 + 1 is computed when k0 <= BIG_N_MINUS_2, and BIG_N_MINUS_2 < U256::MAX. I can change the unwrap_or to expect too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an expect with the reasoning above would be clearer.

let d = Zeroizing::new(pk.to_be_bytes());
break SigningKey::from_bytes(d.as_ref()).unwrap();
}

*counter = counter
Expand Down