Repository: zmap/zlint
Pull request #287 (2019-06-06 00:28:17)

expand subject DN informational only lint
Expanded the check for no metadata only in subject DN to check for subject DN fields containing no characters in a-Z0-9 or outside of ascii table. This catches more than just checking for ".", "-", " ". Also remove separate checks for serial and domainComponent as they are part of pkix.Names so separate checking was redundant.


This will help zlint catch issues that CABLint catches today such as 
https://crt.sh/?id=106177929&opt=zlint,cablint,x509lint 
https://crt.sh/?id=134328239&opt=cablint,zlint
https://crt.sh/?id=26408912&opt=cablint,zlint

As far as I know there aren't any combinations of characters outside of a-Z0-9 and UTF8 which wouldn't be described as metadata only. This assumption needs some scrutiny though.
The new check is still susceptible to UTF8 runes metadata only.


cpu: @tadukurow Can you resolve conflicts with master when you have a chance? I'll endeavour to give this a :mag: in the next few days.
tadukurow: @cpu did you mean rebase off master? because I don't see any conflicts
cpu: > @cpu did you mean rebase off master? because I don't see any conflicts

Yup! I mis-read the Github status message. On the repositories I spend most of my time we don't have the "Require branches to be up to date before merging" checkbox checked and so only conflicts cause that kind of status block. Zlint requires all PRs be current with master.



Patch:
diff --git a/lints/lint_subject_contains_noninformational_value.go b/lints/lint_subject_contains_noninformational_value.go
index 797c2fce..b2d4d91b 100644
--- a/lints/lint_subject_contains_noninformational_value.go
+++ b/lints/lint_subject_contains_noninformational_value.go
@@ -24,7 +24,7 @@ be used.
 **********************************************************************************************************************/
 
 import (
-	"strings"
+	"fmt"
 
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
@@ -41,28 +41,17 @@ func (l *illegalChar) CheckApplies(c *x509.Certificate) bool {
 }
 
 func (l *illegalChar) Execute(c *x509.Certificate) *LintResult {
-	domain := c.Subject.DomainComponent
-	serial := c.Subject.SerialNumber
-	names := c.Subject.Names
-	for _, j := range names {
-		tempStr, ok := j.Value.(string)
+	for _, j := range c.Subject.Names {
+		value, ok := j.Value.(string)
 		if !ok {
-			continue //TODO: change this?
+			continue
 		}
-		if tempStr == "-" || tempStr == "." || tempStr == " " {
-			return &LintResult{Status: Error}
-		}
-	}
-	if serial == "-" || serial == "." || serial == " " {
-		return &LintResult{Status: Error}
-	}
-	for _, j := range domain {
-		if strings.Compare(j, "-") == 0 ||
-			strings.Compare(j, ".") == 0 ||
-			strings.Compare(j, " ") == 0 {
-			return &LintResult{Status: Error}
+
+		if !checkAlphaNumericOrUTF8Present(value) {
+			return &LintResult{Status: Error, Details: fmt.Sprintf("found only metadata %s in subjectDN attribute %s", value, j.Type.String())}
 		}
 	}
+
 	return &LintResult{Status: Pass}
 }
 
@@ -76,3 +65,15 @@ func init() {
 		Lint:          &illegalChar{},
 	})
 }
+
+// checkAlphaNumericOrUTF8Present checks if input string contains at least one occurrence of [a-Z0-9] or
+// a UTF8 rune outside of ascii table
+func checkAlphaNumericOrUTF8Present(input string) bool {
+	for _, r := range input {
+		if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r > 127 {
+			return true
+		}
+	}
+
+	return false
+}


Test patch:
diff --git a/lints/lint_subject_contains_noninformational_value_test.go b/lints/lint_subject_contains_noninformational_value_test.go
index f042d557..fb68c453 100644
--- a/lints/lint_subject_contains_noninformational_value_test.go
+++ b/lints/lint_subject_contains_noninformational_value_test.go
@@ -18,20 +18,118 @@ import (
 	"testing"
 )
 
-func TestSubjectNotInformational(t *testing.T) {
-	inputPath := "../testlint/testCerts/illegalChar.pem"
-	expected := Error
-	out := Lints["e_subject_contains_noninformational_value"].Execute(ReadCertificate(inputPath))
-	if out.Status != expected {
-		t.Errorf("%s: expected %s, got %s", inputPath, expected, out.Status)
+func TestSubjectInformational(t *testing.T) {
+	testCases := []struct {
+		name      string
+		inputPath string
+		result    LintStatus
+	}{
+		{
+			name:      "simple all legal",
+			inputPath: "../testlint/testCerts/legalChar.pem",
+			result:    Pass,
+		},
+		{
+			name:      "subject with metadata only",
+			inputPath: "../testlint/testCerts/illegalChar.pem",
+			result:    Error,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			out := Lints["e_subject_contains_noninformational_value"].Execute(ReadCertificate(tc.inputPath))
+			if out.Status != tc.result {
+				t.Errorf("%s: expected %s, got %s", tc.inputPath, tc.result, out.Status)
+			}
+		})
 	}
 }
 
-func TestSubjectInformational(t *testing.T) {
-	inputPath := "../testlint/testCerts/legalChar.pem"
-	expected := Pass
-	out := Lints["e_subject_contains_noninformational_value"].Execute(ReadCertificate(inputPath))
-	if out.Status != expected {
-		t.Errorf("%s: expected %s, got %s", inputPath, expected, out.Status)
+func TestCheckAlphaNumericOrUTF8Present(t *testing.T) {
+	testCases := []struct {
+		name   string
+		input  string
+		result bool
+	}{
+		{
+			name:   "ascii lowercase",
+			input:  "aa",
+			result: true,
+		},
+		{
+			name:   "ascii uppercase",
+			input:  "AA",
+			result: true,
+		},
+		{
+			name:   "ascii numbers",
+			input:  "123",
+			result: true,
+		},
+		{
+			name:   "ascii start with metadata",
+			input:  "-- abc3",
+			result: true,
+		},
+		{
+			name:   "ascii end with metadata",
+			input:  "abc3 ..",
+			result: true,
+		},
+		{
+			name:   "UTF8",
+			input:  "テスト",
+			result: true,
+		},
+		{
+			name:   "UTF8 start with metadata",
+			input:  "?? テスト",
+			result: true,
+		},
+		{
+			name:   "UTF8 end with metadata",
+			input:  "テスト ??",
+			result: true,
+		},
+		{
+			name:   "-",
+			input:  "-",
+			result: false,
+		},
+		{
+			name:   "**",
+			input:  "**",
+			result: false,
+		},
+		{
+			name:   "...",
+			input:  "...",
+			result: false,
+		},
+		{
+			name:   "- -",
+			input:  "- -",
+			result: false,
+		},
+		{
+			name:   " -",
+			input:  " -",
+			result: false,
+		},
+		{
+			name:   " ",
+			input:  " ",
+			result: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := checkAlphaNumericOrUTF8Present(tc.input)
+			if result != tc.result {
+				t.Errorf("expected check to be %v, got %v", tc.result, result)
+			}
+		})
 	}
 }