diff options
author | Justus Winter <justus@sequoia-pgp.org> | 2022-05-11 13:43:41 +0200 |
---|---|---|
committer | Justus Winter <justus@sequoia-pgp.org> | 2022-06-08 10:59:35 +0200 |
commit | f685b70e2daf534bf2560bdfa909d620b5b41237 (patch) | |
tree | e13f220515db32d08a18da0db00d157f26e0ec95 /openpgp/src/cert | |
parent | 0bc5fa77df3c9dc325afd824b413841b51a4e7ec (diff) |
openpgp: Accept unknown packets in production rules.
- We validate certificate structures based on packet tags. In the
past, this lead to problems where a secret key packet was parsed
to an unknown packet because the secret bits were malformed. This
lead to a crash in the generated parser because it was assuming to
see a secret key packet, but got an unknown packet.
- This was changed in cd5eb82edfb326d7cbde29ee105f9f88e045c240 so
that we validate the certificate structure based on packet kinds,
i.e. we would only consider a packet a secret key packet if we
managed to parse it into one. However, this caused the parser to
be overly strict, causing problems with forward compatibility, and
the parser to return an Error::MalformedCert instead of an
Error::UnsupportedCert (see #170).
- Return to validating on packet tags, but make the parser code
aware that we may have parsed some packets (like secret key
packets) to unknown packets.
- This effectively reverts commit
cd5eb82edfb326d7cbde29ee105f9f88e045c240.
- Fixes #170.
Diffstat (limited to 'openpgp/src/cert')
-rw-r--r-- | openpgp/src/cert/parser/low_level/grammar.lalrpop | 80 | ||||
-rw-r--r-- | openpgp/src/cert/parser/low_level/grammar_util.rs | 12 | ||||
-rw-r--r-- | openpgp/src/cert/parser/low_level/lexer.rs | 21 | ||||
-rw-r--r-- | openpgp/src/cert/parser/low_level/mod.rs | 2 |
4 files changed, 79 insertions, 36 deletions
diff --git a/openpgp/src/cert/parser/low_level/grammar.lalrpop b/openpgp/src/cert/parser/low_level/grammar.lalrpop index d81ff6e3..59462c8c 100644 --- a/openpgp/src/cert/parser/low_level/grammar.lalrpop +++ b/openpgp/src/cert/parser/low_level/grammar.lalrpop @@ -1,16 +1,21 @@ // -*- mode: Rust; -*- +use std::convert::TryInto; + use crate::Error; +use crate::packet::Any; use crate::packet::Signature; use crate::packet::UserID; use crate::packet::UserAttribute; use crate::packet::{key, Key}; use crate::packet::Unknown; use crate::Packet; +use crate::policy::HashAlgoSecurity::SecondPreImageResistance; use crate::cert::prelude::*; use crate::cert::parser::low_level::lexer; use crate::cert::parser::low_level::lexer::{Token, Component}; +use crate::cert::parser::low_level::grammar_util::*; use crate::cert::ComponentBundles; use crate::cert::bundle::{ PrimaryKeyBundle, @@ -157,7 +162,7 @@ OptionalComponents: Option<Vec<Component>> = { Component: Option<Component> = { <key:Subkey> <sigs:OptionalSignatures> => { match key { - Some(key) => { + Some(Ok(key)) => { let sigs = sigs.unwrap(); let sec = key.hash_algo_security(); @@ -171,13 +176,22 @@ Component: Option<Component> = { other_revocations: vec![], })) }, + Some(Err(u)) => Some(Component::UnknownBundle(UnknownBundle { + component: u, + hash_algo_security: SecondPreImageResistance, + self_signatures: vec![], + certifications: sigs.unwrap_or_default(), + attestations: vec![], + self_revocations: vec![], + other_revocations: vec![], + })), // Just validating a cert... None => None, } }, <u:UserID> <sigs:OptionalSignatures> => { match u { - Some(u) => { + Some(Ok(u)) => { let sigs = sigs.unwrap(); let sec = u.hash_algo_security(); @@ -191,13 +205,22 @@ Component: Option<Component> = { other_revocations: vec![], })) }, + Some(Err(u)) => Some(Component::UnknownBundle(UnknownBundle { + component: u, + hash_algo_security: SecondPreImageResistance, + self_signatures: vec![], + certifications: sigs.unwrap_or_default(), + attestations: vec![], + self_revocations: vec![], + other_revocations: vec![], + })), // Just validating a cert... None => None, } }, <u:UserAttribute> <sigs:OptionalSignatures> => { match u { - Some(u) => { + Some(Ok(u)) => { let sigs = sigs.unwrap(); let sec = u.hash_algo_security(); @@ -211,6 +234,15 @@ Component: Option<Component> = { other_revocations: vec![], })) }, + Some(Err(u)) => Some(Component::UnknownBundle(UnknownBundle { + component: u, + hash_algo_security: SecondPreImageResistance, + self_signatures: vec![], + certifications: sigs.unwrap_or_default(), + attestations: vec![], + self_revocations: vec![], + other_revocations: vec![], + })), // Just validating a cert... None => None, } @@ -237,59 +269,55 @@ Component: Option<Component> = { }, } -Subkey: Option<Key<key::PublicParts, key::SubordinateRole>> = { +Subkey: Option<PacketOrUnknown<Key<key::PublicParts, key::SubordinateRole>>> = { <t:PUBLIC_SUBKEY> => { - match t.into() { - Some(Packet::PublicSubkey(key)) => Some(key), + match Option::<Packet>::from(t) { + Some(p) => Some(p.downcast().map_err( + |p| p.try_into().expect("infallible for unknown and this packet"))), // Just validating a cert... None => None, - Some(pkt) => - unreachable!("Expected public subkey packet, got {:?}", pkt), } }, <t:SECRET_SUBKEY> => { - match t.into() { - Some(Packet::SecretSubkey(key)) => Some(key.parts_into_public()), + match Option::<Packet>::from(t) { + Some(p) => Some(Any::<key::SecretSubkey>::downcast(p) + .map_err( + |p| p.try_into().expect("infallible for unknown and this packet")) + .map(|sk| sk.parts_into_public())), // Just validating a cert... None => None, - Some(pkt) => - unreachable!("Expected secret subkey packet, got {:?}", pkt), } }, } -UserID: Option<UserID> = { +UserID: Option<PacketOrUnknown<UserID>> = { <t:USERID> => { - match t.into() { - Some(Packet::UserID(u)) => Some(u), + match Option::<Packet>::from(t) { + Some(p) => Some(p.downcast().map_err( + |p| p.try_into().expect("infallible for unknown and this packet"))), // Just validating a cert... None => None, - Some(pkt) => - unreachable!("Expected user id packet, got {:?}", pkt), } }, } -UserAttribute: Option<UserAttribute> = { +UserAttribute: Option<PacketOrUnknown<UserAttribute>> = { <t:USER_ATTRIBUTE> => { - match t.into() { - Some(Packet::UserAttribute(u)) => Some(u), + match Option::<Packet>::from(t) { + Some(p) => Some(p.downcast().map_err( + |p| p.try_into().expect("infallible for unknown and this packet"))), // Just validating a cert... None => None, - Some(pkt) => - unreachable!("Expected user attribute packet, got {:?}", pkt), } }, } Unknown: Option<Unknown> = { <t:UNKNOWN> => { - match t.into() { - Some(Packet::Unknown(u)) => Some(u), + match Option::<Packet>::from(t) { + Some(p) => p.try_into().ok(), // Just validating a cert... None => None, - Some(pkt) => - unreachable!("Expected unknown packet, got {:?}", pkt), } }, } diff --git a/openpgp/src/cert/parser/low_level/grammar_util.rs b/openpgp/src/cert/parser/low_level/grammar_util.rs new file mode 100644 index 00000000..8d4aa529 --- /dev/null +++ b/openpgp/src/cert/parser/low_level/grammar_util.rs @@ -0,0 +1,12 @@ +//! Contains utility functions and definitions for the grammar. + +use crate::{ + packet::Unknown, +}; + +/// A local alias that is either a concrete packet body or a (likely +/// Unknown) packet. +/// +/// This is used in the parser grammar, but cannot be defined there. +/// lalrpop's parser doesn't allow polymorphic type aliases. +pub type PacketOrUnknown<T> = std::result::Result<T, Unknown>; diff --git a/openpgp/src/cert/parser/low_level/lexer.rs b/openpgp/src/cert/parser/low_level/lexer.rs index 29639271..0838d2b1 100644 --- a/openpgp/src/cert/parser/low_level/lexer.rs +++ b/openpgp/src/cert/parser/low_level/lexer.rs @@ -114,16 +114,17 @@ impl From<Token> for Option<Packet> { impl From<Packet> for Option<Token> { fn from(p: Packet) -> Self { - match p { - p @ Packet::PublicKey(_) => Some(Token::PublicKey(Some(p))), - p @ Packet::SecretKey(_) => Some(Token::SecretKey(Some(p))), - p @ Packet::PublicSubkey(_) => Some(Token::PublicSubkey(Some(p))), - p @ Packet::SecretSubkey(_) => Some(Token::SecretSubkey(Some(p))), - p @ Packet::UserID(_) => Some(Token::UserID(Some(p))), - p @ Packet::UserAttribute(_) => Some(Token::UserAttribute(Some(p))), - p @ Packet::Signature(_) => Some(Token::Signature(Some(p))), - p @ Packet::Trust(_) => Some(Token::Trust(Some(p))), - p @ Packet::Unknown(_) => Some(Token::Unknown(p.tag(), Some(p))), + match p.tag() { + Tag::PublicKey => Some(Token::PublicKey(Some(p))), + Tag::SecretKey => Some(Token::SecretKey(Some(p))), + Tag::PublicSubkey => Some(Token::PublicSubkey(Some(p))), + Tag::SecretSubkey => Some(Token::SecretSubkey(Some(p))), + Tag::UserID => Some(Token::UserID(Some(p))), + Tag::UserAttribute => Some(Token::UserAttribute(Some(p))), + Tag::Signature => Some(Token::Signature(Some(p))), + Tag::Trust => Some(Token::Trust(Some(p))), + t @ Tag::Unknown(_) => Some(Token::Unknown(t, Some(p))), + t @ Tag::Private(_) => Some(Token::Unknown(t, Some(p))), _ => None, } } diff --git a/openpgp/src/cert/parser/low_level/mod.rs b/openpgp/src/cert/parser/low_level/mod.rs index eef79987..79f620d3 100644 --- a/openpgp/src/cert/parser/low_level/mod.rs +++ b/openpgp/src/cert/parser/low_level/mod.rs @@ -13,6 +13,8 @@ lalrpop_util::lalrpop_mod!( "/cert/parser/low_level/grammar.rs" ); +pub(crate) mod grammar_util; + pub(crate) use self::lexer::Token; pub(crate) use self::lexer::Lexer; |