Skip to content

Commit

Permalink
[FAB-3051] Input validation on CSR fields
Browse files Browse the repository at this point in the history 
RFC 3280 specifies upper bounds for fields in the
CSR. This change set enforces those bounds by
doing input validation on the CSR fields. These
checks will also prevents authentication issues
when the DN gets too long and token authenication
errors out.

For more information, see [FAB-3051]

Change-Id: I170b372f2732b147b3ae0811b071ad4328533d0e
Signed-off-by: Saad Karim <[email protected]>
  • Loading branch information
Saad Karim committed Jul 24, 2017
1 parent 4e5c55f commit e03673c
Show file tree
Hide file tree
Showing 2 changed files with 192 additions and 4 deletions.
128 changes: 128 additions & 0 deletions lib/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"crypto/x509"
"fmt"
"io/ioutil"
"math/rand"
"net/http"
"os"
"path"
Expand All @@ -31,6 +32,7 @@ import (
"testing"
"time"

"github.com/cloudflare/cfssl/csr"
"github.com/hyperledger/fabric-ca/api"
. "github.com/hyperledger/fabric-ca/lib"
"github.com/hyperledger/fabric-ca/lib/dbutil"
Expand Down Expand Up @@ -1304,6 +1306,122 @@ func TestNewUserRegistryMySQL(t *testing.T) {
}
}

func TestCSRInputLengthCheck(t *testing.T) {
t.Log("Testing CSR input length check")
os.RemoveAll("../testdata/msp/")
os.RemoveAll(rootDir)
defer os.RemoveAll("../testdata/msp/")
defer os.RemoveAll(rootDir)

server := TestGetServer(rootPort, rootDir, "", -1, t)
if server == nil {
return
}
longCN := randSeq(65)
err := server.RegisterBootstrapUser(longCN, "pass", "")
if err != nil {
t.Errorf("Failed to register bootstrap user: %s", err)
}
err = server.Start()
if err != nil {
t.Fatalf("Server start failed: %s", err)
}
defer server.Stop()

// Passing case: all value are of appropriate length
client := getRootClient()
csr1 := api.CSRInfo{
CN: "test",
Names: []csr.Name{
csr.Name{
C: "US",
ST: "North Carolina",
L: "Raleigh",
O: "Hyperledger",
OU: "Fabric",
},
csr.Name{
C: "CA",
},
},
SerialNumber: "123abc",
}
_, err = client.Enroll(&api.EnrollmentRequest{
Name: "admin",
Secret: "adminpw",
CSR: &csr1,
})
if err != nil {
t.Error("Failed to enroll user in passing case: ", err)
}

// Failing case: CN is greater than 64 characters
badCSR := &api.CSRInfo{
CN: longCN,
}
_, err = client.Enroll(&api.EnrollmentRequest{
Name: longCN,
Secret: "pass",
CSR: badCSR,
})
if assert.Error(t, err, fmt.Sprint("Number of characters for CN is greater than the maximum limit, should have resulted in an error")) {
assert.Contains(t, err.Error(), "CN")
}

// CSRs that test failing cases for other fields in the CSR
badCSRs := map[string]*api.CSRInfo{
"country": &api.CSRInfo{
Names: []csr.Name{
csr.Name{
C: randSeq(3),
},
},
},
"locality": &api.CSRInfo{
Names: []csr.Name{
csr.Name{
L: randSeq(129),
},
},
},
"state": &api.CSRInfo{
Names: []csr.Name{
csr.Name{
ST: randSeq(129),
},
},
},
"organization": &api.CSRInfo{
Names: []csr.Name{
csr.Name{
O: randSeq(65),
},
},
},
"organizational unit": &api.CSRInfo{
Names: []csr.Name{
csr.Name{
OU: randSeq(65),
},
},
},
"serial number": &api.CSRInfo{
SerialNumber: randSeq(65),
},
}

for name, badCSR := range badCSRs {
_, err = client.Enroll(&api.EnrollmentRequest{
Name: "admin",
Secret: "adminpw",
CSR: badCSR,
})
if assert.Error(t, err, fmt.Sprintf("Number of characters for '%s' is greater than the maximum limit, should have resulted in an error", name)) {
assert.Contains(t, err.Error(), name)
}
}
}

func TestEnd(t *testing.T) {
os.Remove("../testdata/ca-cert.pem")
os.Remove("../testdata/ca-key.pem")
Expand Down Expand Up @@ -1510,3 +1628,13 @@ intermediate:
t.Fatalf("Failed to create ca1.yaml: %s", err)
}
}

var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")

func randSeq(n int) string {
b := make([]rune, n)
for i := range b {
b[i] = letters[rand.Intn(len(letters))]
}
return string(b)
}
68 changes: 64 additions & 4 deletions lib/serverenroll.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,26 @@ import (
"github.com/hyperledger/fabric-ca/util"
)

const (
commonNameLength = 64
serialNumberLength = 64
countryNameLength = 2
localityNameLength = 128
stateOrProvinceNameLength = 128
organizationNameLength = 64
organizationalUnitNameLength = 64
)

var (
// The X.509 BasicConstraints object identifier (RFC 5280, 4.2.1.9)
basicConstraintsOID = asn1.ObjectIdentifier{2, 5, 29, 19}
basicConstraintsOID = asn1.ObjectIdentifier{2, 5, 29, 19}
commonNameOID = asn1.ObjectIdentifier{2, 5, 4, 3}
serialNumberOID = asn1.ObjectIdentifier{2, 5, 4, 5}
countryOID = asn1.ObjectIdentifier{2, 5, 4, 6}
localityOID = asn1.ObjectIdentifier{2, 5, 4, 7}
stateOID = asn1.ObjectIdentifier{2, 5, 4, 8}
organizationOID = asn1.ObjectIdentifier{2, 5, 4, 10}
organizationalUnitOID = asn1.ObjectIdentifier{2, 5, 4, 11}
)

// newEnrollHandler is the constructor for the enroll handler
Expand Down Expand Up @@ -111,7 +128,7 @@ func (sh *signHandler) handle(w http.ResponseWriter, r *http.Request) error {
// Make any authorization checks needed, depending on the contents
// of the CSR (Certificate Signing Request)
enrollmentID := r.Header.Get(enrollmentIDHdrName)
err = sh.csrAuthCheck(&req.SignRequest, enrollmentID, r)
err = sh.csrChecks(&req.SignRequest, enrollmentID, r)
if err != nil {
return err
}
Expand All @@ -136,7 +153,9 @@ func (sh *signHandler) handle(w http.ResponseWriter, r *http.Request) error {
// of the CSR (Certificate Signing Request).
// In particular, if the request is for an intermediate CA certificate,
// the caller must have the "hf.IntermediateCA" attribute.
func (sh *signHandler) csrAuthCheck(req *signer.SignRequest, enrollmentID string, r *http.Request) error {
// Also check to see that CSR values do not exceed the character limit
// as specified in RFC 3280, page 103.
func (sh *signHandler) csrChecks(req *signer.SignRequest, enrollmentID string, r *http.Request) error {
// Decode and parse the request into a CSR so we can make checks
caname := r.Header.Get(caHdrName)
block, _ := pem.Decode([]byte(req.Request))
Expand Down Expand Up @@ -173,6 +192,47 @@ func (sh *signHandler) csrAuthCheck(req *signer.SignRequest, enrollmentID string
}
}
}
log.Debug("CSR request received")
log.Debug("CSR authorization check passed")
return csrInputLengthCheck(csrReq)
}

// Checks to make sure that character limits are not exceeded for CSR fields
func csrInputLengthCheck(req *x509.CertificateRequest) error {
log.Debug("Checking CSR fields to make sure that they do not exceed maximum character limits")

for _, n := range req.Subject.Names {
value := n.Value.(string)
switch {
case n.Type.Equal(commonNameOID):
if len(value) > commonNameLength {
return fmt.Errorf("The CN '%s' exceeds the maximum character limit of %d", value, commonNameLength)
}
case n.Type.Equal(serialNumberOID):
if len(value) > serialNumberLength {
return fmt.Errorf("The serial number '%s' exceeds the maximum character limit of %d", value, serialNumberLength)
}
case n.Type.Equal(organizationalUnitOID):
if len(value) > organizationalUnitNameLength {
return fmt.Errorf("The organizational unit name '%s' exceeds the maximum character limit of %d", value, organizationalUnitNameLength)
}
case n.Type.Equal(organizationOID):
if len(value) > organizationNameLength {
return fmt.Errorf("The organization name '%s' exceeds the maximum character limit of %d", value, organizationNameLength)
}
case n.Type.Equal(countryOID):
if len(value) > countryNameLength {
return fmt.Errorf("The country name '%s' exceeds the maximum character limit of %d", value, countryNameLength)
}
case n.Type.Equal(localityOID):
if len(value) > localityNameLength {
return fmt.Errorf("The locality name '%s' exceeds the maximum character limit of %d", value, localityNameLength)
}
case n.Type.Equal(stateOID):
if len(value) > stateOrProvinceNameLength {
return fmt.Errorf("The state name '%s' exceeds the maximum character limit of %d", value, stateOrProvinceNameLength)
}
}
}

return nil
}

0 comments on commit e03673c

Please sign in to comment.