From 47e052031c8fe1e07fabc12a374f5d823f6d6c39 Mon Sep 17 00:00:00 2001 From: btoews Date: Mon, 29 Jan 2024 10:56:13 -0700 Subject: [PATCH] don't assume canonical encoding of macaroons/caveats What is this, SAML? Re-encoding for signature validation opens the door for vulnerabilities. It also is a pain to make sure that implementations in other languages encode macaroons/caveats identically to Go. --- caveat_set.go | 162 +++++++++++++++++---- caveat_test.go | 12 +- caveats_test.go | 10 +- cid.go | 2 +- internal/test-vectors/test_vectors.go | 6 +- internal/test-vectors/test_vectors_test.go | 1 - macaroon.go | 77 +++++----- macaroon_test.go | 18 +-- resset/if_present.go | 2 +- 9 files changed, 202 insertions(+), 88 deletions(-) diff --git a/caveat_set.go b/caveat_set.go index 66c5ab5..b5be09d 100644 --- a/caveat_set.go +++ b/caveat_set.go @@ -1,6 +1,7 @@ package macaroon import ( + "bytes" "encoding/json" "errors" "fmt" @@ -11,7 +12,22 @@ import ( // CaveatSet is how a set of caveats is serailized/encoded. type CaveatSet struct { - Caveats []Caveat + // The caveats encoded as necessary for macaroon signing/verification. + // Access this via the PackedCaveats() method. When signing, each caveat is + // encoded as a msgpack array containing the caveat type and the caveat body + // (the same as a caveat set containing the one caveat). These encodings are + // preserved from caveats sets we decode. This accounts for any variability + // from different msgpack libraries in different languages. + packedCaveats [][]byte + + // it's possible to build a CaveatSet that can't be msgpack encoded. We + // track any error here so we can return it when something tries to get the + // msgpack representation. This is mostly a thing when JSON decoding + // unregistered caveat types. + packErr error + + // Decoded caveats. Access this via the Caveats() method. + caveats []Caveat } var ( @@ -20,12 +36,14 @@ var ( _ msgpack.Marshaler = (*CaveatSet)(nil) ) -// Create a new CaveatSet comprised of the specified caveats. +// NewCaveatSet creates a new CaveatSet comprised of the specified caveats. func NewCaveatSet(caveats ...Caveat) *CaveatSet { - return &CaveatSet{append([]Caveat{}, caveats...)} + c := &CaveatSet{caveats: []Caveat{}, packedCaveats: [][]byte{}} + c.Add(caveats...) + return c } -// Decodes a set of serialized caveats. +// DecodeCaveats decodes a set of serialized caveats. func DecodeCaveats(buf []byte) (*CaveatSet, error) { cavs := new(CaveatSet) @@ -36,6 +54,16 @@ func DecodeCaveats(buf []byte) (*CaveatSet, error) { return cavs, nil } +// Caveats are the decoded caveats. +func (c *CaveatSet) Caveats() []Caveat { + return c.caveats +} + +// PackedCaveats are the caveats, msgpack encoded for signing/verification. +func (c *CaveatSet) PackedCaveats() ([][]byte, error) { + return c.packedCaveats, c.packErr +} + // Validates that the caveat set permits the specified accesses. func (c *CaveatSet) Validate(accesses ...Access) error { return Validate(c, accesses...) @@ -58,7 +86,7 @@ func Validate[A Access](cs *CaveatSet, accesses ...A) error { func (c *CaveatSet) validateAccess(access Access) error { var err error - for _, caveat := range c.Caveats { + for _, caveat := range c.caveats { if IsAttestation(caveat) { continue } @@ -72,7 +100,7 @@ func (c *CaveatSet) validateAccess(access Access) error { // GetCaveats gets any caveats of type T, including those nested within // IfPresent caveats. func GetCaveats[T Caveat](c *CaveatSet) (ret []T) { - for _, cav := range c.Caveats { + for _, cav := range c.caveats { if typed, ok := cav.(T); ok { ret = append(ret, typed) } @@ -89,18 +117,25 @@ func (c CaveatSet) MarshalMsgpack() ([]byte, error) { return encode(c) } +// cavPrefix is the msgpack tag indicating an array of length 2. +const cavPrefix = byte(0x92) + // Implements msgpack.CustomEncoder func (c CaveatSet) EncodeMsgpack(enc *msgpack.Encoder) error { - if err := enc.EncodeArrayLen(len(c.Caveats) * 2); err != nil { - return err + if c.packErr != nil { + return c.packErr } - for _, cav := range c.Caveats { - if err := enc.EncodeUint(uint64(cav.CaveatType())); err != nil { - return err - } + // TODO: resize enc buffer, since we know how much we're going to write? + + if err := enc.EncodeArrayLen(len(c.packedCaveats) * 2); err != nil { + return err + } - if err := enc.Encode(cav); err != nil { + for _, b := range c.packedCaveats { + // each CaveatBytes is itself a caveat set (msgpack array len=2). Skip + // the array tag when encoding them all together. + if err := enc.Encode(msgpack.RawMessage(b[1:])); err != nil { return err } } @@ -118,37 +153,94 @@ func (c *CaveatSet) DecodeMsgpack(dec *msgpack.Decoder) error { return errors.New("bad caveat container") } - nCavs := aLen / 2 + c.caveats = make([]Caveat, aLen/2) + c.packedCaveats = make([][]byte, aLen/2) - if c.Caveats == nil { - c.Caveats = make([]Caveat, 0, nCavs) - } + for i := 0; i < aLen/2; i++ { + rawTyp, err := dec.DecodeRaw() + if err != nil { + return err + } - for i := 0; i < nCavs; i++ { - t, err := dec.DecodeUint() + var typ CaveatType + if err := msgpack.Unmarshal(rawTyp, &typ); err != nil { + return err + } + + rawCav, err := dec.DecodeRaw() if err != nil { return err } - cav := typeToCaveat(CaveatType(t)) - if err := dec.Decode(cav); err != nil { + c.caveats[i] = typeToCaveat(CaveatType(typ)) + if err := msgpack.Unmarshal(rawCav, c.caveats[i]); err != nil { return err } - c.Caveats = append(c.Caveats, cav) + c.packedCaveats[i] = make([]byte, 0, 1+len(rawTyp)+len(rawCav)) + c.packedCaveats[i] = append(c.packedCaveats[i], cavPrefix) + c.packedCaveats[i] = append(c.packedCaveats[i], rawTyp...) + c.packedCaveats[i] = append(c.packedCaveats[i], rawCav...) } return nil } +func (c *CaveatSet) Add(caveats ...Caveat) { + c.caveats = append(c.caveats, caveats...) + + if c.packErr != nil { + return + } + + for _, cav := range caveats { + packed, err := packCaveat(cav) + if err != nil { + c.packedCaveats = nil + c.packErr = err + + return + } + + c.packedCaveats = append(c.packedCaveats, packed) + } +} + +func (c *CaveatSet) addWithPacked(cav Caveat, packed []byte) { + c.caveats = append(c.caveats, cav) + if c.packErr == nil { + c.packedCaveats = append(c.packedCaveats, packed) + } +} + +func packCaveat(cav Caveat) ([]byte, error) { + enc := msgpack.GetEncoder() + defer msgpack.PutEncoder(enc) + + var buf bytes.Buffer + configEncoder(enc, &buf) + + if err := enc.Encode(msgpack.RawMessage([]byte{cavPrefix})); err != nil { + return nil, err + } + if err := enc.EncodeUint(uint64(cav.CaveatType())); err != nil { + return nil, err + } + if err := enc.Encode(cav); err != nil { + return nil, err + } + + return buf.Bytes(), nil +} + func (c CaveatSet) MarshalJSON() ([]byte, error) { var ( - jcavs = make([]jsonCaveat, len(c.Caveats)) + jcavs = make([]jsonCaveat, len(c.caveats)) err error ) - for i := range c.Caveats { - ct := c.Caveats[i].CaveatType() + for i := range c.caveats { + ct := c.caveats[i].CaveatType() cts := caveatTypeToString(ct) if cts == "" { return nil, fmt.Errorf("unregistered caveat type: %d", ct) @@ -158,7 +250,7 @@ func (c CaveatSet) MarshalJSON() ([]byte, error) { Type: cts, } - if jcavs[i].Body, err = json.Marshal(c.Caveats[i]); err != nil { + if jcavs[i].Body, err = json.Marshal(c.caveats[i]); err != nil { return nil, err } } @@ -173,14 +265,26 @@ func (c *CaveatSet) UnmarshalJSON(b []byte) error { return err } - c.Caveats = make([]Caveat, len(jcavs)) + c.caveats = make([]Caveat, 0, len(jcavs)) + c.packedCaveats = make([][]byte, 0, len(jcavs)) for i := range jcavs { t := caveatTypeFromString(jcavs[i].Type) - c.Caveats[i] = typeToCaveat(t) - if err := json.Unmarshal(jcavs[i].Body, &c.Caveats[i]); err != nil { + cav := typeToCaveat(t) + if err := json.Unmarshal(jcavs[i].Body, &cav); err != nil { return err } + c.caveats = append(c.caveats, cav) + + if c.packErr == nil { + if packed, err := packCaveat(cav); err != nil { + c.packErr = err + c.packedCaveats = nil + } else { + c.packedCaveats = append(c.packedCaveats, packed) + } + } + } return nil diff --git a/caveat_test.go b/caveat_test.go index 2eb9f68..f340538 100644 --- a/caveat_test.go +++ b/caveat_test.go @@ -16,17 +16,17 @@ func TestCaveatRegistry(t *testing.T) { ) assert.NoError(t, json.Unmarshal(j1, cs)) - assert.Equal(t, 1, len(cs.Caveats)) - assert.Equal(t, c, cs.Caveats[0]) + assert.Equal(t, 1, len(cs.Caveats())) + assert.Equal(t, c, cs.Caveats()[0]) RegisterCaveatJSONAlias(cavTestParentResource, "Foobar") t.Cleanup(func() { unegisterCaveatJSONAlias("Foobar") }) assert.NoError(t, json.Unmarshal(j1, cs)) - assert.Equal(t, 1, len(cs.Caveats)) - assert.Equal(t, c, cs.Caveats[0]) + assert.Equal(t, 1, len(cs.Caveats())) + assert.Equal(t, c, cs.Caveats()[0]) assert.NoError(t, json.Unmarshal(j2, cs)) - assert.Equal(t, 1, len(cs.Caveats)) - assert.Equal(t, c, cs.Caveats[0]) + assert.Equal(t, 1, len(cs.Caveats())) + assert.Equal(t, c, cs.Caveats()[0]) } diff --git a/caveats_test.go b/caveats_test.go index f1f45da..7090d3b 100644 --- a/caveats_test.go +++ b/caveats_test.go @@ -165,9 +165,9 @@ func TestUnregisteredCaveatJSON(t *testing.T) { cs2 := NewCaveatSet() err = json.Unmarshal(b, cs2) assert.NoError(t, err) - assert.Equal(t, 1, len(cs2.Caveats)) + assert.Equal(t, 1, len(cs2.Caveats())) - uc, ok := cs2.Caveats[0].(*UnregisteredCaveat) + uc, ok := cs2.Caveats()[0].(*UnregisteredCaveat) assert.True(t, ok) assert.Equal(t, cavMyUnregistered, uc.Type) @@ -207,9 +207,9 @@ func TestUnregisteredCaveatMsgpack(t *testing.T) { cs2, err := DecodeCaveats(b) assert.NoError(t, err) - assert.Equal(t, 1, len(cs2.Caveats)) + assert.Equal(t, 1, len(cs2.Caveats())) - uc, ok := cs2.Caveats[0].(*UnregisteredCaveat) + uc, ok := cs2.Caveats()[0].(*UnregisteredCaveat) assert.True(t, ok) assert.Equal(t, cavMyUnregistered, uc.Type) @@ -235,7 +235,7 @@ func TestUnregisteredCaveatMsgpack(t *testing.T) { cs3, err := DecodeCaveats(b2) assert.NoError(t, err) - assert.Equal(t, 1, len(cs3.Caveats)) + assert.Equal(t, 1, len(cs3.Caveats())) mucs := GetCaveats[*myUnregistered](cs3) assert.Equal(t, 1, len(mucs)) assert.Equal(t, c, mucs[0]) diff --git a/cid.go b/cid.go index af20a9f..3588206 100644 --- a/cid.go +++ b/cid.go @@ -53,5 +53,5 @@ func dischargeTicket(ka EncryptionKey, location string, ticket []byte, issueProo return nil, nil, err } - return tWire.Caveats.Caveats, dm, nil + return tWire.Caveats.Caveats(), dm, nil } diff --git a/internal/test-vectors/test_vectors.go b/internal/test-vectors/test_vectors.go index d2dfb36..2470d66 100644 --- a/internal/test-vectors/test_vectors.go +++ b/internal/test-vectors/test_vectors.go @@ -33,7 +33,7 @@ func main() { } v.KID = keyFingerprint(v.Key) - for _, c := range caveats.Caveats { + for _, c := range caveats.Caveats() { m, _ := macaroon.New(v.KID, v.Location, v.Key) // put attestations in discharge tokens @@ -57,7 +57,7 @@ func main() { aBaseTok, _ := aBase.Encode() aBaseHdr := macaroon.ToAuthorizationHeader(otherTok, aBaseTok, otherTok) v.Attenuation[aBaseHdr] = map[string]string{} - for _, c := range caveats.Caveats { + for _, c := range caveats.Caveats() { cpy := ptr(*aBase) cpy.UnsafeCaveats = *macaroon.NewCaveatSet() cpy.Add(c) @@ -66,7 +66,7 @@ func main() { v.Attenuation[aBaseHdr][base64.StdEncoding.EncodeToString(cavsPacked)] = macaroon.ToAuthorizationHeader(otherTok, cpyEnc, otherTok) } - for _, c := range caveats.Caveats { + for _, c := range caveats.Caveats() { v.Caveats[c.Name()] = pack(c) } diff --git a/internal/test-vectors/test_vectors_test.go b/internal/test-vectors/test_vectors_test.go index 107856a..91ec6a9 100644 --- a/internal/test-vectors/test_vectors_test.go +++ b/internal/test-vectors/test_vectors_test.go @@ -9,7 +9,6 @@ import ( ) func TestCaveatSerialization(t *testing.T) { - b, err := json.Marshal(caveats) assert.NoError(t, err) diff --git a/macaroon.go b/macaroon.go index f88e349..a036f6c 100644 --- a/macaroon.go +++ b/macaroon.go @@ -128,9 +128,7 @@ func encode(v interface{}) ([]byte, error) { enc := msgpack.GetEncoder() defer msgpack.PutEncoder(enc) - enc.Reset(buf) - enc.UseArrayEncodedStructs(true) - enc.UseCompactInts(true) + configEncoder(enc, buf) if err := enc.Encode(v); err != nil { return nil, err @@ -138,6 +136,12 @@ func encode(v interface{}) ([]byte, error) { return buf.Bytes(), nil } +func configEncoder(enc *msgpack.Encoder, buf *bytes.Buffer) { + enc.Reset(buf) + enc.UseArrayEncodedStructs(true) + enc.UseCompactInts(true) +} + // New creates a new token given a key-id string (which can // be any opaque string and doesn't need to be cryptographically // random or anything; the key-id is how you're going to relate @@ -209,8 +213,8 @@ func (m *Macaroon) Add(caveats ...Caveat) error { return errors.New("can't add caveats to finalized proof") } - var err error - if caveats, err = m.dedup(caveats); err != nil { + caveats, err := m.dedup(caveats) + if err != nil { return fmt.Errorf("deduplicating caveats: %w", err) } @@ -234,15 +238,13 @@ func (m *Macaroon) Add(caveats ...Caveat) error { seen3P[c3p.Location] = true } - m.UnsafeCaveats.Caveats = append(m.UnsafeCaveats.Caveats, caveat) - - opc, err := NewCaveatSet(caveat).MarshalMsgpack() + packed, err := packCaveat(caveat) if err != nil { return fmt.Errorf("mint: encode caveat: %w", err) } - m.Tail = sign(SigningKey(m.Tail), opc) - + m.UnsafeCaveats.addWithPacked(caveat, packed) + m.Tail = sign(SigningKey(m.Tail), packed) } return nil @@ -253,28 +255,26 @@ func (m *Macaroon) Add(caveats ...Caveat) error { // // TODO: ignore caveats that are subsets of existing caveats func (m *Macaroon) dedup(caveats []Caveat) ([]Caveat, error) { - seen := make(map[string]bool, len(m.UnsafeCaveats.Caveats)) - - for _, cav := range m.UnsafeCaveats.Caveats { - packed, err := NewCaveatSet(cav).MarshalMsgpack() - if err != nil { - return nil, err - } + mPacked, err := m.UnsafeCaveats.PackedCaveats() + if err != nil { + return nil, err + } - seen[hex.EncodeToString(packed)] = true + seen := make(map[string]bool, len(mPacked)) + for _, packed := range mPacked { + seen[string(packed)] = true } ret := make([]Caveat, 0, len(caveats)) for _, cav := range caveats { - packed, err := NewCaveatSet(cav).MarshalMsgpack() + packed, err := packCaveat(cav) if err != nil { return nil, err } - str := hex.EncodeToString(packed) - if !seen[str] { + if s := string(packed); !seen[s] { + seen[s] = true ret = append(ret, cav) - seen[str] = true } } @@ -341,8 +341,16 @@ func (m *Macaroon) verify(k SigningKey, discharges [][]byte, parentTokenBindingI dischargesToVerify := make([]*verifyParams, 0, len(dischargeByTicket)) thisTokenBindingIds := [][]byte{digest(curMac)} - for _, c := range m.UnsafeCaveats.Caveats { - switch cav := c.(type) { + caveats := m.UnsafeCaveats.Caveats() + caveatsPacked, err := m.UnsafeCaveats.PackedCaveats() + if err != nil { + return nil, err + } + + for i := range caveats { + packed := caveatsPacked[i] + + switch cav := caveats[i].(type) { case *Caveat3P: discharge, ok := dischargeByTicket[string(cav.Ticket)] if !ok { @@ -373,16 +381,11 @@ func (m *Macaroon) verify(k SigningKey, discharges [][]byte, parentTokenBindingI } if !IsAttestation(cav) || trustAttestations { - ret.Caveats = append(ret.Caveats, c) + ret.addWithPacked(cav, packed) } } - opc, err := NewCaveatSet(c).MarshalMsgpack() - if err != nil { - return nil, err - } - - curMac = sign(SigningKey(curMac), opc) + curMac = sign(SigningKey(curMac), packed) thisTokenBindingIds = append(thisTokenBindingIds, digest(curMac)) } @@ -409,7 +412,7 @@ func (m *Macaroon) verify(k SigningKey, discharges [][]byte, parentTokenBindingI trustedDischarge = true } - dcavs, err := d.m.verify( + dcs, err := d.m.verify( d.k, nil, /* don't let them nest yet */ thisTokenBindingIds, @@ -420,7 +423,15 @@ func (m *Macaroon) verify(k SigningKey, discharges [][]byte, parentTokenBindingI return nil, fmt.Errorf("macaroon verify: verify discharge: %w", err) } - ret.Caveats = append(ret.Caveats, dcavs.Caveats...) + dCavs := dcs.Caveats() + dCavsPacked, err := dcs.PackedCaveats() + if err != nil { + return nil, err + } + + for i := range dCavs { + ret.addWithPacked(dCavs[i], dCavsPacked[i]) + } } if m.Nonce.Proof { diff --git a/macaroon_test.go b/macaroon_test.go index ff42045..58ca255 100644 --- a/macaroon_test.go +++ b/macaroon_test.go @@ -188,7 +188,7 @@ func TestMacaroons(t *testing.T) { decoded, err = Decode(encoded) assert.NoError(t, err) - decodedCavs = decoded.UnsafeCaveats.Caveats + decodedCavs = decoded.UnsafeCaveats.Caveats() } requireVerify := func(t *testing.T) { @@ -490,7 +490,7 @@ func TestAttenuate(t *testing.T) { m2, err := decoded.Verify(key, nil, nil) assert.NoError(t, err) - t.Logf("%s", m2) + t.Logf("%#v", m2) } func TestSimple3P(t *testing.T) { @@ -615,25 +615,25 @@ func TestDuplicateCaveats(t *testing.T) { assert.NoError(t, err) assert.NoError(t, m.Add(cavParent(ActionAll, 123))) - assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionAll, 123))) - assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionAll, 123))) - assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 1, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionAll, 234))) - assert.Equal(t, 2, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 2, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionRead, 123))) - assert.Equal(t, 3, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 3, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionRead, 234))) - assert.Equal(t, 4, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 4, len(m.UnsafeCaveats.Caveats())) assert.NoError(t, m.Add(cavParent(ActionAll, 345), cavParent(ActionAll, 345))) - assert.Equal(t, 5, len(m.UnsafeCaveats.Caveats)) + assert.Equal(t, 5, len(m.UnsafeCaveats.Caveats())) } func TestDecodeNonce(t *testing.T) { diff --git a/resset/if_present.go b/resset/if_present.go index 90e8a61..6fa4c87 100644 --- a/resset/if_present.go +++ b/resset/if_present.go @@ -38,7 +38,7 @@ func (c *IfPresent) Prohibits(a macaroon.Access) error { ifBranch bool ) - for _, cc := range c.Ifs.Caveats { + for _, cc := range c.Ifs.Caveats() { // set err if any of the `Ifs` returns nil or a non-errResourceUnspecified error if cErr := cc.Prohibits(ra); !errors.Is(cErr, ErrResourceUnspecified) { err = merr.Append(err, cErr)