Add lint which checks for explicitly encoded critical default values

[alex]

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: pyca/cryptography#6368)

[christopher-henderson]

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?

[alex]

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.

[christopher-henderson]

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.

[alex]

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.

[christopher-henderson]

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
}

[christopher-henderson]

(figured it out, zcrypto/asn1 vomits on it but the standard library likes it just fine)

[christopher-henderson]

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.

[defacto64]

This is now addressed by #839 :)