wfe: reject empty identifiers in new-authz and new-order (letsencrypt#5089)

jsha · web-flow · commit ef955a561a73 · 2020-09-15T09:42:33.000-07:00
Currently these are rejected at the RA. It's nicer to reject them one step earlier. Fixes letsencrypt#5081
diff --git a/wfe/wfe.go b/wfe/wfe.go
@@ -742,17 +742,34 @@ func (wfe *WebFrontEndImpl) NewAuthorization(ctx context.Context, logEvent *web.
 		return
 	}
 
-	var init core.Authorization
-	if err := json.Unmarshal(body, &init); err != nil {
+	var newAuthzRequest struct {
+		Identifier struct {
+			Type  string
+			Value string
+		}
+	}
+	if err := json.Unmarshal(body, &newAuthzRequest); err != nil {
 		wfe.sendError(response, logEvent, probs.Malformed("Error unmarshaling JSON"), err)
 		return
 	}
-	if init.Identifier.Type == identifier.DNS {
-		logEvent.DNSName = init.Identifier.Value
+	if newAuthzRequest.Identifier.Type == "" || newAuthzRequest.Identifier.Value == "" {
+		wfe.sendError(response, logEvent, probs.Malformed("Invalid new-authorization request: missing fields"), nil)
+		return
+	}
+	if newAuthzRequest.Identifier.Type == string(identifier.DNS) {
+		logEvent.DNSName = newAuthzRequest.Identifier.Value
+	} else {
+		wfe.sendError(response, logEvent, probs.Malformed("Invalid new-authorization request: wrong identifier type"), nil)
+		return
 	}
 
 	// Create new authz and return
-	authz, err := wfe.RA.NewAuthorization(ctx, init, currReg.ID)
+	authz, err := wfe.RA.NewAuthorization(ctx, core.Authorization{
+		Identifier: identifier.ACMEIdentifier{
+			Type:  identifier.DNS,
+			Value: newAuthzRequest.Identifier.Value,
+		},
+	}, currReg.ID)
 	if err != nil {
 		wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Error creating new authz"), err)
 		return
diff --git a/wfe/wfe_test.go b/wfe/wfe_test.go
@@ -1775,6 +1775,57 @@ func TestAuthorization500(t *testing.T) {
 
 }
 
+func TestNewAuthorizationEmptyDomain(t *testing.T) {
+	responseWriter := httptest.NewRecorder()
+	wfe, _ := setupWFE(t)
+
+	wfe.NewAuthorization(ctx, newRequestEvent(), responseWriter,
+		makePostRequest(signRequest(t, `{
+		  "resource":"new-authz",
+			"identifier": {
+				"Type": "dns",
+				"Value": ""
+			}
+		}`, wfe.nonceService)))
+	test.AssertUnmarshaledEquals(t,
+		responseWriter.Body.String(),
+		`{"type":"`+probs.V1ErrorNS+`malformed","detail":"Invalid new-authorization request: missing fields","status":400}`)
+}
+
+func TestNewAuthorizationEmptyType(t *testing.T) {
+	responseWriter := httptest.NewRecorder()
+	wfe, _ := setupWFE(t)
+
+	wfe.NewAuthorization(ctx, newRequestEvent(), responseWriter,
+		makePostRequest(signRequest(t, `{
+		  "resource":"new-authz",
+			"identifier": {
+				"Type": "",
+				"Value": "example.com"
+			}
+		}`, wfe.nonceService)))
+	test.AssertUnmarshaledEquals(t,
+		responseWriter.Body.String(),
+		`{"type":"`+probs.V1ErrorNS+`malformed","detail":"Invalid new-authorization request: missing fields","status":400}`)
+}
+
+func TestNewAuthorizationNonDNS(t *testing.T) {
+	responseWriter := httptest.NewRecorder()
+	wfe, _ := setupWFE(t)
+
+	wfe.NewAuthorization(ctx, newRequestEvent(), responseWriter,
+		makePostRequest(signRequest(t, `{
+		  "resource":"new-authz",
+			"identifier": {
+				"Type": "shibboleth",
+				"Value": "example.com"
+			}
+		}`, wfe.nonceService)))
+	test.AssertUnmarshaledEquals(t,
+		responseWriter.Body.String(),
+		`{"type":"`+probs.V1ErrorNS+`malformed","detail":"Invalid new-authorization request: wrong identifier type","status":400}`)
+}
+
 func TestAuthorization(t *testing.T) {
 	wfe, _ := setupWFE(t)
 	mux := wfe.Handler(metrics.NoopRegisterer)
diff --git a/wfe2/wfe.go b/wfe2/wfe.go
@@ -1970,6 +1970,10 @@ func (wfe *WebFrontEndImpl) NewOrder(
 				nil)
 			return
 		}
+		if ident.Value == "" {
+			wfe.sendError(response, logEvent, probs.Malformed("NewOrder request included empty domain name"), nil)
+			return
+		}
 		names[i] = ident.Value
 	}
 
diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go
@@ -2232,6 +2232,11 @@ func TestNewOrder(t *testing.T) {
 			Request:      signAndPost(t, targetPath, signedURL, "foo", 1, wfe.nonceService),
 			ExpectedBody: `{"type":"` + probs.V2ErrorNS + `malformed","detail":"Request payload did not parse as JSON","status":400}`,
 		},
+		{
+			Name:         "POST, empty domain name identifier",
+			Request:      signAndPost(t, targetPath, signedURL, `{"identifiers":[{"type":"dns","value":""}]}`, 1, wfe.nonceService),
+			ExpectedBody: `{"type":"` + probs.V2ErrorNS + `malformed","detail":"NewOrder request included empty domain name","status":400}`,
+		},
 		{
 			Name:         "POST, no identifiers in payload",
 			Request:      signAndPost(t, targetPath, signedURL, "{}", 1, wfe.nonceService),

Original file line number	Diff line number	Diff line change
`@@ -1970,6 +1970,10 @@ func (wfe *WebFrontEndImpl) NewOrder(`
`1970`	`1970`	`nil)`
`1971`	`1971`	`return`
`1972`	`1972`	`}`
	`1973`	`+ if ident.Value == "" {`
	`1974`	`+ wfe.sendError(response, logEvent, probs.Malformed("NewOrder request included empty domain name"), nil)`
	`1975`	`+ return`
	`1976`	`+ }`
`1973`	`1977`	`names[i] = ident.Value`
`1974`	`1978`	`}`
`1975`	`1979`