From f92ac2477b3f857dfe2399a304033d9bcf3d1b3e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 6 Mar 2017 09:59:00 +0100 Subject: [PATCH 1/3] Add test to run into issue with stack.len() --- openssl/src/pkcs12.rs | 11 +++++++++++ openssl/test/keystore-empty-chain.p12 | Bin 0 -> 2514 bytes 2 files changed, 11 insertions(+) create mode 100644 openssl/test/keystore-empty-chain.p12 diff --git a/openssl/src/pkcs12.rs b/openssl/src/pkcs12.rs index 9f014af6..31864802 100644 --- a/openssl/src/pkcs12.rs +++ b/openssl/src/pkcs12.rs @@ -196,6 +196,17 @@ mod test { "c0cbdf7cdd03c9773e5468e1f6d2da7d5cbb1875"); } + #[test] + fn parse_empty_chain() { + let der = include_bytes!("../test/keystore-empty-chain.p12"); + let pkcs12 = Pkcs12::from_der(der).unwrap(); + let parsed = pkcs12.parse("cassandra").unwrap(); + + + assert_eq!(parsed.chain.len(), 0); + assert_eq!(parsed.chain.into_iter().collect::>().len(), 0); + } + #[test] fn create() { let subject_name = "ns.example.com"; diff --git a/openssl/test/keystore-empty-chain.p12 b/openssl/test/keystore-empty-chain.p12 new file mode 100644 index 0000000000000000000000000000000000000000..c39930a5cb4f003f48ab94cc87f9435730540f24 GIT binary patch literal 2514 zcmY+Ec{tPy7sm%PYGy29l6{>Jnwto_x^F7^PKPZobP|CE5VLn=X=W_4eD5|T+WbPU?{wz^ zH>yNRB3)^xvxrw`6y9QzEe&Ir3xrv&)=9$}Ojm zp87&GAzImu81Q6Or6jiZBY>eoBA1%QB6d{Jv>u<1l`Cb;gsPFlEaz{~(}R~?&nn-@ zf``oK!#6k^Q(`9^iuLc2oos8z%QGJ^X{^DhauJ?wl}EB6o1v&Mt_^YYcyTgf=vJJm zOYzw5`(x0ZukQ!rnPyLhV+2AIeG;tdyecnz*875iCFW6Sd^cU=R9r@h*H0N)Ygf(B zT{~hs_tAQ? zpqnyo)W(i0oo#W8v>#H{aCl(CnniSv5GL~TgsSL>+!;~GPHcVhyxEg9HT!O3hQ#Jz zK!UCDrnL>b^@p-9j}2QYI6-PF<9Z6No+Db2n^T0^>y--ZWJ6qyHSx5N^6d3FuJCVUF?n@%oB|JI?3ZQbL*|$Ze;*FF>BxCJY-woQq6^ya!)N6N!p{3 z!p}`FRd28+K^8$xOVhFLEPed+(#WMRzZ+n^UayqqfFZmN<#=v(RdARpWpN^-x);v% zX|2z6%W-|~o5uJRyU3~uhxqS}h}x1=EDHuFy$F#x)DBNnS8Rak=hQa1vczEgV;uo^ zSuX504fh%!tBmaN>C4KVQ4k^Le9H~Zy4<|gI#2BL($kTOa}L|ePk8vCxsdT{tR3be(;s@>0W_9e_qr=&0gJjch7@p zd24}H+J)D<$vVY37Jlc;^mJwk?iu#&8V5x#Eb|$iMxciAU;hknf`l{<+zg+Ysx!SQ z^kRmxSvtV;CaARArawN8oL28z@o;wTU8uRq1f4E7lAp}q8u;!*Wt${UiZip!lK6ej zppxlTYMR=H#}LH)+%G6^Hr7gwrvsg}h5tEp|H3TePt2kl$o8!oe@SY%#wJnH#R#F)9L!J5Mgk0_ z+A>$>d{P6qA0&iEAz6Nc&ac8-BMdsViNc z08tj)MOm~O*mj`P?Ym0EGqfmP+JDX+=jO^c&>~w>S2gzeG5j>WP~>e8sp2 zni?~rZG{z0z4Ww`jr0upA9Zpk*(RmjMkjtY+yn=@WhkL~-CUicfURyv&_xO;N@y2l zW9c^7s2esbFg=+rD^iM&iACCCN{q&*M>yQ0ij9ZrgCNy+qo3NGU%M5a(9Li+<)d|CUPcXZR9SLr)CR-ZO`nPVgUY+B&=tKL3}*w9s)m zi{{pf19HH=mC%1br%b9fz;mjvhG?c{o4L`7=RTOzBpLhj5+vp9y5pHW#E1J+kK2cA z*IBTj>qeI|de>hfmXnWAvs%`Xr^`NV7`v8?UA zgketBHv*Dh9>yESCZu548vQ;#S@`kr)g`-TgTT2;(dRnyOrvts=GMt@2}07C`M4Z6 zCsol(_oTozF{<%d9croZo4Re&wF~brEk?8#RkTyy=BQ5@ZKJ`Cg~P5Cw<_%)F+F9t z7W-F)$LWOCeU)Aky!00+))0>#-*m{Vqb1qpLvDba}yh0 zTEHEbx;=aAPbHEM@_f`dRPP Date: Mon, 6 Mar 2017 10:14:39 +0100 Subject: [PATCH 2/3] Fix for empty stacks The culprit is that `sk_num(stack)` can return -1 as c_int if there is no stack allocated. Previously, thanks to unsafe casts, this would result in a isize::max() for len() and iteration size if there was no stack. Now this case is handled specifically, which fixes the issue. --- openssl/src/stack.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/openssl/src/stack.rs b/openssl/src/stack.rs index 268afde7..d9795a51 100644 --- a/openssl/src/stack.rs +++ b/openssl/src/stack.rs @@ -116,7 +116,7 @@ pub struct IntoIter { impl IntoIter { fn stack_len(&self) -> c_int { - unsafe { OPENSSL_sk_num(self.stack as *mut _) } + safe_stack_size(self.stack as *mut _) as c_int } } @@ -154,6 +154,15 @@ impl ExactSizeIterator for IntoIter {} pub struct StackRef(Opaque, PhantomData); +fn safe_stack_size(stack: *mut OPENSSL_STACK) -> usize { + let l = unsafe { OPENSSL_sk_num(stack) as isize }; + if l < 0 { + 0 + } else { + l as usize + } +} + impl ForeignTypeRef for StackRef { type CType = T::StackType; } @@ -165,7 +174,7 @@ impl StackRef { /// Returns the number of items in the stack pub fn len(&self) -> usize { - unsafe { OPENSSL_sk_num(self.as_stack()) as usize } + safe_stack_size(self.as_stack()) } pub fn iter(&self) -> Iter { From 463db85110658db729c722e6f2ef63fc67b4788b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 7 Mar 2017 07:39:25 +0100 Subject: [PATCH 3/3] Don't allow Stacks to be allocated with a null-ptr The latter must be seen as undefined behaviour, as it will cause the `sk_num` function to return -1 to indicate the error, which causes all kinds of issues. Thus there now is a panic to abort the program if stacks are initialized with a null-ptr, and special handling of that case when decoding a Pkcs file. --- openssl/src/pkcs12.rs | 9 +++++++-- openssl/src/stack.rs | 15 ++++----------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/openssl/src/pkcs12.rs b/openssl/src/pkcs12.rs index 31864802..31aae536 100644 --- a/openssl/src/pkcs12.rs +++ b/openssl/src/pkcs12.rs @@ -42,7 +42,12 @@ impl Pkcs12Ref { let pkey = PKey::from_ptr(pkey); let cert = X509::from_ptr(cert); - let chain = Stack::from_ptr(chain); + + let chain = if chain.is_null() { + try!(Stack::new()) + } else { + Stack::from_ptr(chain) + }; Ok(ParsedPkcs12 { pkey: pkey, @@ -80,6 +85,7 @@ impl Pkcs12 { pub struct ParsedPkcs12 { pub pkey: PKey, pub cert: X509, + // FIXME Make this Option in the next breaking release pub chain: Stack, } @@ -202,7 +208,6 @@ mod test { let pkcs12 = Pkcs12::from_der(der).unwrap(); let parsed = pkcs12.parse("cassandra").unwrap(); - assert_eq!(parsed.chain.len(), 0); assert_eq!(parsed.chain.into_iter().collect::>().len(), 0); } diff --git a/openssl/src/stack.rs b/openssl/src/stack.rs index d9795a51..6ac8264c 100644 --- a/openssl/src/stack.rs +++ b/openssl/src/stack.rs @@ -86,6 +86,8 @@ impl ForeignType for Stack { #[inline] unsafe fn from_ptr(ptr: *mut T::StackType) -> Stack { + assert!(!ptr.is_null(), "Must not instantiate a Stack from a null-ptr - use Stack::new() in \ + that case"); Stack(ptr) } @@ -116,7 +118,7 @@ pub struct IntoIter { impl IntoIter { fn stack_len(&self) -> c_int { - safe_stack_size(self.stack as *mut _) as c_int + unsafe { OPENSSL_sk_num(self.stack as *mut _) } } } @@ -154,15 +156,6 @@ impl ExactSizeIterator for IntoIter {} pub struct StackRef(Opaque, PhantomData); -fn safe_stack_size(stack: *mut OPENSSL_STACK) -> usize { - let l = unsafe { OPENSSL_sk_num(stack) as isize }; - if l < 0 { - 0 - } else { - l as usize - } -} - impl ForeignTypeRef for StackRef { type CType = T::StackType; } @@ -174,7 +167,7 @@ impl StackRef { /// Returns the number of items in the stack pub fn len(&self) -> usize { - safe_stack_size(self.as_stack()) + unsafe { OPENSSL_sk_num(self.as_stack()) as usize } } pub fn iter(&self) -> Iter {