From d4cbdc2beb0460918d9c358e1bc39a5dbaa67b09 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Wed, 8 Feb 2023 10:37:04 +0100 Subject: feat(tvix/nix-compat/nixhash): support BASE64_NOPAD SRI Nix accepts SRI hashes that are missing their padding characters in base64, as seen in https://github.com/NixOS/nixpkgs/blob/7e49471316373c471a3bf4b78c130ebc907ae2d2/pkgs/development/libraries/kerberos/krb5.nix . It only seems to work in the SRI case, not with `sha256` being set to a (nopad) base64 string. Add regression tests for this, and document why we don't want to support *additional* characters afterwards. Reported in https://b.tvl.fyi/issues/252 Change-Id: I9ffc2b417501b426ced1894a9cbf95ff5f0e5159 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8037 Reviewed-by: Alyssa Ross Tested-by: BuildkiteCI --- tvix/nix-compat/src/nixhash.rs | 80 ++++++++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 19 deletions(-) diff --git a/tvix/nix-compat/src/nixhash.rs b/tvix/nix-compat/src/nixhash.rs index 7fe657d76240..0a7b1b137531 100644 --- a/tvix/nix-compat/src/nixhash.rs +++ b/tvix/nix-compat/src/nixhash.rs @@ -1,3 +1,4 @@ +use data_encoding::{BASE64, BASE64_NOPAD}; use std::fmt::Display; use thiserror::Error; @@ -111,11 +112,9 @@ pub fn from_str(s: &str, algo_str: Option<&str>) -> Result { n if n == nixbase32::encode_len(expected_digest_len) => { nixbase32::decode(s.as_ref()).map_err(Error::InvalidBase32Encoding) } - n if n == data_encoding::BASE64.encode_len(expected_digest_len) => { - data_encoding::BASE64 - .decode(s.as_ref()) - .map_err(Error::InvalidBase64Encoding) - } + n if n == BASE64.encode_len(expected_digest_len) => BASE64 + .decode(s.as_ref()) + .map_err(Error::InvalidBase64Encoding), _ => { // another length than what we expected from the passed hash algo // try to parse as SRI @@ -150,6 +149,7 @@ pub fn from_str(s: &str, algo_str: Option<&str>) -> Result { /// Contrary to the SRI spec, Nix doesn't support SRI strings with multiple hashes, /// only supports sha256 and sha512 from the spec, and supports sha1 and md5 /// additionally. +/// It also accepts SRI strings where the base64 has an with invalid padding. pub fn from_sri_str(s: &str) -> Result { // try to find the first occurence of "-" let idx = s.as_bytes().iter().position(|&e| e == b'-'); @@ -164,22 +164,32 @@ pub fn from_sri_str(s: &str) -> Result { let algo: HashAlgo = s[..idx].try_into()?; // the rest should be the digest (as Nix doesn't support more than one hash in an SRI string). - let digest_str = &s[idx + 1..]; - - // verify the digest length matches what we'd expect from the hash function. - // This will also reject strings with more than one hash, because the length won't match. - if digest_str.as_bytes().len() != data_encoding::BASE64.encode_len(hash_algo_length(&algo)) { - return Err(Error::InvalidEncodedDigestLength( - digest_str.as_bytes().len(), + let encoded_digest = &s[idx + 1..]; + let actual_len = encoded_digest.as_bytes().len(); + + // verify the digest length matches what we'd expect from the hash function, + // and then either try decoding as BASE64 or BASE64_NOPAD. + // This will also reject SRI strings with more than one hash, because the length won't match + if actual_len == BASE64.encode_len(hash_algo_length(&algo)) { + let digest: Vec = BASE64 + .decode(encoded_digest.as_bytes()) + .map_err(Error::InvalidBase64Encoding)?; + Ok(NixHash { digest, algo }) + } else if actual_len == BASE64_NOPAD.encode_len(hash_algo_length(&algo)) { + let digest: Vec = BASE64_NOPAD + .decode(encoded_digest.as_bytes()) + .map_err(Error::InvalidBase64Encoding)?; + Ok(NixHash { digest, algo }) + } else { + // NOTE: As of now, we reject SRI hashes containing additional + // characters (which upstream Nix seems to simply truncate), as + // there's no occurence of this is in nixpkgs. + // It most likely should also be a bug in Nix. + Err(Error::InvalidEncodedDigestLength( + encoded_digest.as_bytes().len(), algo, - )); + )) } - - // decode the base64 string - let digest: Vec = data_encoding::BASE64 - .decode(digest_str.as_bytes()) - .map_err(Error::InvalidBase64Encoding)?; - Ok(NixHash { digest, algo }) } #[cfg(test)] @@ -355,4 +365,36 @@ mod tests { fn from_sri_str_unsupported_multiple() { nixhash::from_sri_str("sha256-ngth6szLtC1IJIYyz3lhftzL8SkrJkqPyPve+dGqa1Y= sha512-q0DQvjVB8HdLungV0T0QsDJS6W6V99u07pmjtDHCFmL9aXGgIBYOOYSKpfMFub4PeHJ7KweJ458STSHpK4857w==").expect_err("must fail"); } + + /// Nix also accepts SRI strings with missing padding, but only in case the + /// string is expressed as SRI, so it still needs to have a `sha256-` prefix. + /// + /// This both seems to work if it is passed with and without specifying the + /// hash algo out-of-band (hash = "sha256-…" or sha256 = "sha256-…") + /// + /// Passing the same broken base64 string, but not as SRI, while passing + /// the hash algo out-of-band does not work. + #[test] + fn sha256_broken_padding() { + let broken_base64 = "fgIr3TyFGDAXP5+qoAaiMKDg/a1MlT6Fv/S/DaA24S8"; + // if padded with a trailing '=' + let expected_digest = vec![ + 0x7e, 0x02, 0x2b, 0xdd, 0x3c, 0x85, 0x18, 0x30, 0x17, 0x3f, 0x9f, 0xaa, 0xa0, 0x06, + 0xa2, 0x30, 0xa0, 0xe0, 0xfd, 0xad, 0x4c, 0x95, 0x3e, 0x85, 0xbf, 0xf4, 0xbf, 0x0d, + 0xa0, 0x36, 0xe1, 0x2f, + ]; + + // passing hash algo out of band should succeed + let nix_hash = nixhash::from_str(&format!("sha256-{}", &broken_base64), Some("sha256")) + .expect("must succeed"); + assert_eq!(&expected_digest, &nix_hash.digest); + + // not passing hash algo out of band should succeed + let nix_hash = + nixhash::from_str(&format!("sha256-{}", &broken_base64), None).expect("must succeed"); + assert_eq!(&expected_digest, &nix_hash.digest); + + // not passing SRI, but hash algo out of band should fail + nixhash::from_str(&broken_base64, Some("sha256")).expect_err("must fail"); + } } -- cgit 1.4.1