Skip to content

Reject duplicate JSON members when parsing signatures #252

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.

Sign up for GitHub

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

Merged
mtrmac merged 5 commits into from
Mar 14, 2017
Merged

Reject duplicate JSON members when parsing signatures #252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c813f58
Add paranoidUnmarshalJSONObjectExactFields
mtrmac Mar 13, 2017
89af51f
Use paranoidUnmarshalJSONObjectExactFields in untrustedSignature.Stri…
mtrmac Mar 13, 2017
95908ee
Remove no longer used JSON helpers
mtrmac Mar 13, 2017
b6cea84
Fix TestPolicyUnmarshalJSON
mtrmac Mar 13, 2017
3994e56
Use paranoidUnmarshalJSONObjectExactFields in policy.json parsing
mtrmac Mar 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 22 additions & 58 deletions signature/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,64 +14,6 @@ func (err jsonFormatError) Error() string {
return string(err)
}

// validateExactMapKeys returns an error if the keys of m are not exactly expectedKeys, which must be pairwise distinct
func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) error {
if len(m) != len(expectedKeys) {
return jsonFormatError("Unexpected keys in a JSON object")
}

for _, k := range expectedKeys {
if _, ok := m[k]; !ok {
return jsonFormatError(fmt.Sprintf("Key %s missing in a JSON object", k))
}
}
// Assuming expectedKeys are pairwise distinct, we know m contains len(expectedKeys) different values in expectedKeys.
return nil
}

// int64Field returns a member fieldName of m, if it is an int64, or an error.
func int64Field(m map[string]interface{}, fieldName string) (int64, error) {
untyped, ok := m[fieldName]
if !ok {
return -1, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
}
f, ok := untyped.(float64)
if !ok {
return -1, jsonFormatError(fmt.Sprintf("Field %s is not a number", fieldName))
}
v := int64(f)
if float64(v) != f {
return -1, jsonFormatError(fmt.Sprintf("Field %s is not an integer", fieldName))
}
return v, nil
}

// mapField returns a member fieldName of m, if it is a JSON map, or an error.
func mapField(m map[string]interface{}, fieldName string) (map[string]interface{}, error) {
untyped, ok := m[fieldName]
if !ok {
return nil, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
}
v, ok := untyped.(map[string]interface{})
if !ok {
return nil, jsonFormatError(fmt.Sprintf("Field %s is not a JSON object", fieldName))
}
return v, nil
}

// stringField returns a member fieldName of m, if it is a string, or an error.
func stringField(m map[string]interface{}, fieldName string) (string, error) {
untyped, ok := m[fieldName]
if !ok {
return "", jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
}
v, ok := untyped.(string)
if !ok {
return "", jsonFormatError(fmt.Sprintf("Field %s is not a string", fieldName))
}
return v, nil
}

// paranoidUnmarshalJSONObject unmarshals data as a JSON object, but failing on the slightest unexpected aspect
// (including duplicated keys, unrecognized keys, and non-matching types). Uses fieldResolver to
// determine the destination for a field value, which should return a pointer to the destination if valid, or nil if the key is rejected.
Expand Down Expand Up @@ -122,3 +64,25 @@ func paranoidUnmarshalJSONObject(data []byte, fieldResolver func(string) interfa
}
return nil
}

// paranoidUnmarshalJSONObject unmarshals data as a JSON object, but failing on the slightest unexpected aspect
// (including duplicated keys, unrecognized keys, and non-matching types). Each of the fields in exactFields
// must be present exactly once, and none other fields are accepted.
func paranoidUnmarshalJSONObjectExactFields(data []byte, exactFields map[string]interface{}) error {
seenKeys := map[string]struct{}{}
Copy link
Member

@runcom runcom Mar 13, 2017

Choose a reason for hiding this comment

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

It might be more idiomatic, but a map[string]bool is better suited here and allows to do

If seenkeys[i]...

W/o playing around with !ok below

Copy link
Collaborator Author

@mtrmac mtrmac Mar 14, 2017

Choose a reason for hiding this comment

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

I somewhat prefer the struct{} to make the type closer to the true type (set of string), see also earlier conversation in #223 , and perhaps earlier.

I don’t feel all that strongly about this, but if you can live with it, I’ll merge as is.

if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
if valuePtr, ok := exactFields[key]; ok {
seenKeys[key] = struct{}{}
return valuePtr
}
return nil
}); err != nil {
return err
}
for key := range exactFields {
if _, ok := seenKeys[key]; !ok {
return jsonFormatError(fmt.Sprintf(`Key "%s" missing in a JSON object`, key))
}
}
return nil
}
129 changes: 42 additions & 87 deletions signature/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package signature

import (
"encoding/json"
"fmt"
"math"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -24,91 +22,6 @@ func x(m mSI, fields ...string) mSI {
return m
}

func TestValidateExactMapKeys(t *testing.T) {
// Empty map and keys
err := validateExactMapKeys(mSI{})
assert.NoError(t, err)

// Success
err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "b", "a")
assert.NoError(t, err)

// Extra map keys
err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "a")
assert.Error(t, err)

// Extra expected keys
err = validateExactMapKeys(mSI{"a": 1}, "b", "a")
assert.Error(t, err)

// Unexpected key values
err = validateExactMapKeys(mSI{"a": 1}, "b")
assert.Error(t, err)
}

func TestInt64Field(t *testing.T) {
// Field not found
_, err := int64Field(mSI{"a": "x"}, "b")
assert.Error(t, err)

// Field has a wrong type
_, err = int64Field(mSI{"a": "string"}, "a")
assert.Error(t, err)

for _, value := range []float64{
0.5, // Fractional input
math.Inf(1), // Infinity
math.NaN(), // NaN
} {
_, err = int64Field(mSI{"a": value}, "a")
assert.Error(t, err, fmt.Sprintf("%f", value))
}

// Success
// The float64 type has 53 bits of effective precision, so ±1FFFFFFFFFFFFF is the
// range of integer values which can all be represented exactly (beyond that,
// some are representable if they are divisible by a high enough power of 2,
// but most are not).
for _, value := range []int64{0, 1, -1, 0x1FFFFFFFFFFFFF, -0x1FFFFFFFFFFFFF} {
testName := fmt.Sprintf("%d", value)
v, err := int64Field(mSI{"a": float64(value), "b": nil}, "a")
require.NoError(t, err, testName)
assert.Equal(t, value, v, testName)
}
}

func TestMapField(t *testing.T) {
// Field not found
_, err := mapField(mSI{"a": mSI{}}, "b")
assert.Error(t, err)

// Field has a wrong type
_, err = mapField(mSI{"a": 1}, "a")
assert.Error(t, err)

// Success
// FIXME? We can't use mSI as the type of child, that type apparently can't be converted to the raw map type.
child := map[string]interface{}{"b": mSI{}}
m, err := mapField(mSI{"a": child, "b": nil}, "a")
require.NoError(t, err)
assert.Equal(t, child, m)
}

func TestStringField(t *testing.T) {
// Field not found
_, err := stringField(mSI{"a": "x"}, "b")
assert.Error(t, err)

// Field has a wrong type
_, err = stringField(mSI{"a": 1}, "a")
assert.Error(t, err)

// Success
s, err := stringField(mSI{"a": "x", "b": nil}, "a")
require.NoError(t, err)
assert.Equal(t, "x", s)
}

// implementsUnmarshalJSON is a minimalistic type used to detect that
// paranoidUnmarshalJSONObject uses the json.Unmarshaler interface of resolved
// pointers.
Expand Down Expand Up @@ -180,3 +93,45 @@ func TestParanoidUnmarshalJSONObject(t *testing.T) {
assert.Error(t, err, input)
}
}

func TestParanoidUnmarshalJSONObjectExactFields(t *testing.T) {
var stringValue string
var float64Value float64
var rawValue json.RawMessage
var unmarshallCalled implementsUnmarshalJSON
exactFields := map[string]interface{}{
"string": &stringValue,
"float64": &float64Value,
"raw": &rawValue,
"unmarshaller": &unmarshallCalled,
}

// Empty object
err := paranoidUnmarshalJSONObjectExactFields([]byte(`{}`), map[string]interface{}{})
require.NoError(t, err)

// Success
err = paranoidUnmarshalJSONObjectExactFields([]byte(`{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`), exactFields)
require.NoError(t, err)
assert.Equal(t, "a", stringValue)
assert.Equal(t, 3.5, float64Value)
assert.Equal(t, json.RawMessage(`{"a":"b"}`), rawValue)
assert.Equal(t, implementsUnmarshalJSON(true), unmarshallCalled)

// Various kinds of invalid input
for _, input := range []string{
``, // Empty input
`&`, // Entirely invalid JSON
`1`, // Not an object
`{&}`, // Invalid key JSON
`{1:1}`, // Key not a string
`{"string": "a", "string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Duplicate key
`{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true, "thisisunknown", 1}`, // Unknown key
`{"string": &, "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Invalid value JSON
`{"string": 1, "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Type mismatch
`{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}{}`, // Extra data after object
} {
err := paranoidUnmarshalJSONObjectExactFields([]byte(input), exactFields)
assert.Error(t, err, input)
}
}
84 changes: 19 additions & 65 deletions signature/policy_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,8 @@ var _ json.Unmarshaler = (*prInsecureAcceptAnything)(nil)
func (pr *prInsecureAcceptAnything) UnmarshalJSON(data []byte) error {
*pr = prInsecureAcceptAnything{}
var tmp prInsecureAcceptAnything
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
}); err != nil {
return err
}
Expand Down Expand Up @@ -290,13 +285,8 @@ var _ json.Unmarshaler = (*prReject)(nil)
func (pr *prReject) UnmarshalJSON(data []byte) error {
*pr = prReject{}
var tmp prReject
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
}); err != nil {
return err
}
Expand Down Expand Up @@ -465,25 +455,16 @@ func (pr *prSignedBaseLayer) UnmarshalJSON(data []byte) error {
*pr = prSignedBaseLayer{}
var tmp prSignedBaseLayer
var baseLayerIdentity json.RawMessage
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
case "baseLayerIdentity":
return &baseLayerIdentity
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
"baseLayerIdentity": &baseLayerIdentity,
}); err != nil {
return err
}

if tmp.Type != prTypeSignedBaseLayer {
return InvalidPolicyFormatError(fmt.Sprintf("Unexpected policy requirement type \"%s\"", tmp.Type))
}
if baseLayerIdentity == nil {
return InvalidPolicyFormatError(fmt.Sprintf("baseLayerIdentity not specified"))
}
bli, err := newPolicyReferenceMatchFromJSON(baseLayerIdentity)
if err != nil {
return err
Expand Down Expand Up @@ -541,13 +522,8 @@ var _ json.Unmarshaler = (*prmMatchExact)(nil)
func (prm *prmMatchExact) UnmarshalJSON(data []byte) error {
*prm = prmMatchExact{}
var tmp prmMatchExact
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
}); err != nil {
return err
}
Expand Down Expand Up @@ -576,13 +552,8 @@ var _ json.Unmarshaler = (*prmMatchRepoDigestOrExact)(nil)
func (prm *prmMatchRepoDigestOrExact) UnmarshalJSON(data []byte) error {
*prm = prmMatchRepoDigestOrExact{}
var tmp prmMatchRepoDigestOrExact
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
}); err != nil {
return err
}
Expand Down Expand Up @@ -611,13 +582,8 @@ var _ json.Unmarshaler = (*prmMatchRepository)(nil)
func (prm *prmMatchRepository) UnmarshalJSON(data []byte) error {
*prm = prmMatchRepository{}
var tmp prmMatchRepository
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
}); err != nil {
return err
}
Expand Down Expand Up @@ -656,15 +622,9 @@ var _ json.Unmarshaler = (*prmExactReference)(nil)
func (prm *prmExactReference) UnmarshalJSON(data []byte) error {
*prm = prmExactReference{}
var tmp prmExactReference
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
case "dockerReference":
return &tmp.DockerReference
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
"dockerReference": &tmp.DockerReference,
}); err != nil {
return err
}
Expand Down Expand Up @@ -704,15 +664,9 @@ var _ json.Unmarshaler = (*prmExactRepository)(nil)
func (prm *prmExactRepository) UnmarshalJSON(data []byte) error {
*prm = prmExactRepository{}
var tmp prmExactRepository
if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
switch key {
case "type":
return &tmp.Type
case "dockerRepository":
return &tmp.DockerRepository
default:
return nil
}
if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{
"type": &tmp.Type,
"dockerRepository": &tmp.DockerRepository,
}); err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions signature/policy_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) {

// Various allowed modifications to the policy
allowedModificationFns := []func(mSI){
// Delete the map of specific policies
func(v mSI) { delete(v, "specific") },
// Delete the map of transport-specific scopes
func(v mSI) { delete(v, "transports") },
// Use an empty map of transport-specific scopes
func(v mSI) { v["transports"] = map[string]PolicyTransportScopes{} },
}
Expand Down
Loading