zmap
Add lint which checks for explicitly encoded critical default values
From ITU X.690:
11.5 Set and sequence components with default value
The encoding of a set value or sequence value shall not include an encoding for any component value which is equal to its default value.
From RFC 5280:
Extension ::= SEQUENCE {
extnID OBJECT IDENTIFIER,
critical BOOLEAN DEFAULT FALSE,
extnValue OCTET STRING
-- contains the DER encoding of an ASN.1 value
-- corresponding to the extension type identified
-- by extnID
}
therefore critical should never be encoded with a false value.
Some issuing software does do this though, so a lint to capture it would be valuable.
(My proximate motivation: https://github.com/pyca/cryptography/issues/6368)
I'm sorry, @alex, but I do not believe that this is quite feasible in our codebase without some interesting gymnastics. The reason is as follows...
Consider the definition of a pkix.Extension withing the Go standard library (and zcyrpto as well)
// Extension represents the ASN.1 structure of the same name. See RFC
// 5280, section 4.2.
type Extension struct {
Id asn1.ObjectIdentifier
Critical bool `asn1:"optional"`
Value []byte
}
The Critical data member here is a bool value. In Go, non-pointer values may not be uninitialized. That is, if this field was not found within the source data structure then Go will initialize this field to false on our behalf. That is to say, we may encounter a false value, however we do not know whether the field was (correctly) missing and was thus initialized to false via Go's semantics or if the field actually was explicitly encoded with false (the issue that you encountered in pyca).
Take the following Go playground sample...
package main
import (
"fmt"
"crypto/x509/pkix"
)
func main() {
fmt.Println(pkix.Extension{})
}
We could, conceivable, modify zcrypto with the following definition...
type Extension struct {
Id asn1.ObjectIdentifier
Critical *bool `asn1:"optional"`
Value []byte
}
That is, a pointer to a bool which we may then use to infer whether or not the field was explicitly encoded. However, this would be an extremely invasive change as all dependent code would be forced to account for *bool rather than a plain bool.
Alternatively, we could extend the struct to include this information at the time of deserialization.
type Extension struct {
Id asn1.ObjectIdentifier
Critical bool `asn1:"optional"`
CriticalWasEncoded bool
Value []byte
}
@cpu or @zakird could you think of simpler ways to pull off a lint such as this?
My suggestion would be to do the *bool approach, but in an Extension struct that's only used in this lint, and then simply have this lint re-parse data into this structure. That's the least-invasive way I think think to do it.
Would that not involve yet another fork of zcrypto (which itself would require regular pulls from upstream to receive bugfixes), but private to exactly only this lint?
func (l *Lint) Execute(c *x509.Certificate) *lint.LintResult {
c2 := privateX509.ParseCertificate(c.Raw)
for _, ext := range c2.Extensions {
if !*ext {
panic("boo")
}
}
}
That is certainly non-evasive. However, it is taking a thermonuclear device to a worm.
I don't think it'd require a full fork, it's really just
type certificate struct {
TBSCertificate tbsCertificate
SignatureAlgorithm pkix.AlgorithmIdentifier
SignatureValue asn1.BitString
}
type tbsCertificate struct {
Version int `asn1:"optional,explicit,default:0,tag:0"`
SerialNumber *big.Int
SignatureAlgorithm pkix.AlgorithmIdentifier
Issuer asn1.RawValue
Validity validity
Subject asn1.RawValue
PublicKey publicKeyInfo
UniqueId asn1.BitString `asn1:"optional,tag:1"`
SubjectUniqueId asn1.BitString `asn1:"optional,tag:2"`
Extensions []MyExtension `asn1:"optional,explicit,tag:3"`
}
type validity struct {
NotBefore, NotAfter time.Time
}
type publicKeyInfo struct {
Algorithm pkix.AlgorithmIdentifier
PublicKey asn1.BitString
}
And then an encoding/asn.Unmarshal.
That's actually a good point. I'm not getting any luck with the following definition, however, so I'm gonna debug what I'm doing wrong here (I even left the Extension struct the same, but I'm still getting tag mismatch errors).
type certificate struct {
Raw asn1.RawContent
TBSCertificate tbsCertificate
SignatureAlgorithm pkix.AlgorithmIdentifier
SignatureValue asn1.BitString
}
type tbsCertificate struct {
Raw asn1.RawContent
Version int `asn1:"optional,explicit,default:0,tag:0"`
SerialNumber *big.Int
SignatureAlgorithm pkix.AlgorithmIdentifier
Issuer asn1.RawValue
Validity validity
Subject asn1.RawValue
PublicKey publicKeyInfo
UniqueId asn1.BitString `asn1:"optional,tag:1"`
SubjectUniqueId asn1.BitString `asn1:"optional,tag:2"`
Extensions []Extension `asn1:"optional,explicit,tag:3"`
}
type validity struct {
NotBefore, NotAfter time.Time
}
type publicKeyInfo struct {
Raw asn1.RawContent
Algorithm pkix.AlgorithmIdentifier
PublicKey asn1.BitString
}
type Extension struct {
Id asn1.ObjectIdentifier
Critical bool `asn1:"optional"`
Value []byte
}
(figured it out, zcrypto/asn1 vomits on it but the standard library likes it just fine)
Alas, boo. I don't think the asn1 library deals with pointers. Given getUniversalType
// Given a reflected Go type, getUniversalType returns the default tag number
// and expected compound flag.
func getUniversalType(t reflect.Type) (matchAny bool, tagNumber int, isCompound, ok bool) {
switch t {
case rawValueType:
return true, -1, false, true
case objectIdentifierType:
return false, TagOID, false, true
case bitStringType:
return false, TagBitString, false, true
case timeType:
return false, TagUTCTime, false, true
case enumeratedType:
return false, TagEnum, false, true
case bigIntType:
return false, TagInteger, false, true
}
switch t.Kind() {
case reflect.Bool:
return false, TagBoolean, false, true
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return false, TagInteger, false, true
case reflect.Struct:
return false, TagSequence, true, true
case reflect.Slice:
if t.Elem().Kind() == reflect.Uint8 {
return false, TagOctetString, false, true
}
if strings.HasSuffix(t.Name(), "SET") {
return false, TagSet, true, true
}
return false, TagSequence, true, true
case reflect.String:
return false, TagPrintableString, false, true
}
return false, 0, false, false
}
This naturally results in a asn1: structure error: unknown Go type: *bool error when attempting our little workaround.
