Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional x509 banners #305

Closed
wants to merge 9 commits into from

Conversation

psupel-r7
Copy link

@psupel-r7 psupel-r7 commented Dec 10, 2020

Description

Recog updated with fingerprints from Jess

Motivation and Context

More functionality

How Has This Been Tested?

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have updated the documentation accordingly (or changes are not required).
  • I have added tests to cover my changes (or new tests are not required).
  • All new and existing tests passed.

xml/x509_subjects.xml Outdated Show resolved Hide resolved
xml/x509_subjects.xml Outdated Show resolved Hide resolved
xml/x509_subjects.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@tsellers-r7 tsellers-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@dabdine
Copy link
Contributor

dabdine commented Dec 10, 2020

@tsellers-r7 we should probably add tests against the fields for the files in the 'identifiers' dir of this project (such as hw_product.txt).

Also, I know R7 VM expects explicit vendor/product names to trigger downstream checks, so there may be some additional integration testing necessary so these additions/changes don't regress functionality there.

@tsellers-r7
Copy link
Contributor

@dabdine - Yup, after we move the tests from Travis-CI (tests that take 18+ hours to run aren't useful) we can just have it run bin/recog_standardize to check the results.

@psupel-r7 psupel-r7 changed the title DCA-21047 Updated recog Additional x509 banners Dec 11, 2020
Copy link

@rkirk-r7 rkirk-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we still need to port

  • VMWare - we have os.family values, and looks like one regex match is missing
  • iLo - some existing ones but do not look like they match ours

xml/x509_subjects.xml Outdated Show resolved Hide resolved
xml/x509_subjects.xml Outdated Show resolved Hide resolved
xml/x509_subjects.xml Outdated Show resolved Hide resolved
@hdm
Copy link
Contributor

hdm commented Jan 22, 2021

Hello! The EMAILADDRESS change is problematic today because the x509 representation differs by implementation. For example, Go is putting the email address at the end of the line and using the OID instead "EMAILADDRESS".

This comment is relevant:

<!--
  This fingerprint set matches the Subject field of x509 certificates. These x509
  certificates may be sourced from any SSL or TLS service. If a particular system
  has identical subject and issuer fields, the subject field should be preferred.
  The format of the Subject field is built from the x509 distinguished names using
  a specific order. This order matches the Go implementation at the URL:
    https://golang.org/src/crypto/x509/pkix/pkix.go#203
  The ToRDNSequence() function builds the string in reverse order:
        func (n Name) ToRDNSequence() (ret RDNSequence) {
            ret = n.appendRDNs(ret, n.Country, oidCountry)
            ret = n.appendRDNs(ret, n.Province, oidProvince)
            ret = n.appendRDNs(ret, n.Locality, oidLocality)
            ret = n.appendRDNs(ret, n.StreetAddress, oidStreetAddress)
            ret = n.appendRDNs(ret, n.PostalCode, oidPostalCode)
            ret = n.appendRDNs(ret, n.Organization, oidOrganization)
            ret = n.appendRDNs(ret, n.OrganizationalUnit, oidOrganizationalUnit)
            if len(n.CommonName) > 0 {
                ret = n.appendRDNs(ret, []string{n.CommonName}, oidCommonName)
            }
            if len(n.SerialNumber) > 0 {
                ret = n.appendRDNs(ret, []string{n.SerialNumber}, oidSerialNumber)
            }
            for _, atv := range n.ExtraNames {
                ret = append(ret, []AttributeTypeAndValue{atv})
            }
            return ret
        }
    All names are separated by commas and any commas inside a name are escaped with a
    single backslash character. See RFC 2253 for additional details on formatting.
    Practically, most Subjects start with the Common Name (CN=) and then step through
    Organization Unit (OU), Organization (O), and then some level of location, but
    typically Locality (L) and Country (C). Names are guaranteed to be listed in
    the order described above, but may start at any point in the list. For example,
    Subjects may start with a Serial Number (SERIALNUMBER=) or even Extra Names, but
    these are somewhat rare. Keep this name order in mind when working on these
    fingerprints.
    The same constraints also apply to the x509 Issuers (x509_issuers.xml).
  -->

Supporting EMAILADDRESS (and other attributes is great), but we need to agree on how to represent them first and make sure Go, Ruby, Python, Java all agree when building the string.

@hdm
Copy link
Contributor

hdm commented Jan 22, 2021

The relevant Go code for extra name handling:

			oidString := tv.Type.String()
			typeName, ok := attributeTypeNames[oidString]
			if !ok {
				derBytes, err := asn1.Marshal(tv.Value)
				if err == nil {
					s += oidString + "=#" + hex.EncodeToString(derBytes)
					continue // No value escaping necessary.
				}

				typeName = oidString
			}

In practice this looks like:

CN=device.corp.com,OU=VMware Engineering,O=VMware,L=Palo Alto,ST=California,C=US,1.2.840.113549.1.9.1=#0c0f766d636140766d776172652e636f6d

1.2.840.113549.1.9.1 is the OID of the EMAILADDRESS attribute. The value includes the DER bytes, including the Type and Length before the value ([email protected]).

@hdm
Copy link
Contributor

hdm commented Jan 22, 2021

Go also places these LAST where the example above has the EMAILADDRESS first.

Anywho, this gets tricky because we normally use ^ anchors and this will different by language.

@@ -475,7 +483,7 @@
</fingerprint>

<fingerprint pattern="^CN=([a-zA-Z0-9\.\-\_]+),OU=VMware ESX Server Default Certificate,O=VMware\\, Inc,L=Palo Alto,ST=California,C=US$">
<description>VMware ESX</description>
<description>VMWare ESX</description>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will likely be closed, but note for self - this change is incorrect. VMware is correct.

<param pos="0" name="os.device" value="Hypervisor"/>
<param pos="0" name="os.cpe23" value="cpe:/o:vmware:esx:-"/>
<param pos="1" name="host.name"/>
</fingerprint>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already landed elsewhere

@mkienow-r7
Copy link
Contributor

Closing this pull request since x.509 normalization has not been worked out. See issue #360.

@mkienow-r7 mkienow-r7 closed this Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants