Decoding 9 bytes of bad CBOR data can exhaust memory (message/multimsg/multimsg.go)

[x448]

Decoding 9-10 bytes of malformed CBOR data can cause "fatal error: out of memory" and "runtime: out of memory" errors. Only 1 decode attempt is required to cause the error.

cc @cryptix

Relevant Code

message/multimsg/multimsg.go

import (
... 
    "github.com/ugorji/go/codec"
...
)
...
func (mm *MultiMessage) UnmarshalBinary(data []byte) error {
...
    var mh codec.CborHandle
    mh.StructToArray = true
    dec := codec.NewDecoderBytes(data[1:], &mh)
...
    switch mm.tipe {
    case Legacy:
        var msg legacy.StoredMessage
        err := dec.Decode(&msg)
...
    case Gabby:
        var msg ggWithMetadata
        err := dec.Decode(&msg)

🔥 Error (fatal error: out of memory)

For info about CBOR and security, see Section 8 of RFC 7049 (Security Considerations).

For more comparisons, see fxamacker/cbor.

Description and Minimal Reproduction

In October 2013, RFC 7049 Section 8 (CBOR Security Considerations) warned that malformed CBOR data can be used to exhaust system resources.

Resource exhaustion attacks might attempt to lure a decoder into allocating very big data items (strings, arrays, maps) or exhaust the stack depth by setting up deeply nested items. Decoders need to have appropriate resource management to mitigate these attacks.

In September 2019, oasislabs/oasis-core discovered out of memory errors can be caused by tiny CBOR data and traced the problem to ugorji/go (same CBOR library used by cryptoscope/ssb). They fixed the problem by switching to a more secure CBOR library.

In February 2020, smartcontractkit/chainlink reported an issue on pivitoltracker (I don't have login access) and fixed it in a GitHub PR titled "Switch to more secure CBOR library". They were also using same CBOR library as cryptoscope/ssb.

To reproduce, decode 9-byte or 10-byte malformed CBOR data described in Section 8 of RFC 7049.

Examples of malformed CBOR data to achieve this can be found on GitHub since September 2019 (or possibly earlier if you look beyond Go projects).

[cryptix]

Thanks for reporting this. Looks like I need to change as well, then.

[cryptix]

multimsg is only in charge of handling stored (and validated) messages but this is a critical problem for the gabbygrove feed format, which takes user controlled input in CBOR before it does the signature validation.

I will try and test this more and merge it soon, once I also changed the storage layer, for consistency and dependency cleanup.

[cryptix]

TODO: double check custom tag annotation of cypher references works as expected.

[x448]

gabbygrove feed format, which takes user controlled input in CBOR before it does the signature validation

If security-related data is encoded as elements of CBOR map, then you may want to consider using this decoding option to improve security:

• cbor.DupMapKeyEnforcedAPF -- detect and reject duplicate CBOR map keys. However, there's speed+memory cost (770 ns/op -> 1143 ns/op using CBOR Web Token example data).

By comparison, ugorji/go is around 1100 ns/op even without duplicate map key detection so I think this decoding option is worth considering.

Example using cbor.DupMapKeyEnforcedAPF and cbor.IndefLengthForbidden:

import (
    "github.com/fxamacker/cbor/v2"
)

var (
    fastDecMode = cbor.DecOptions{}.DecMode();    

    safeDecOptions = cbor.DecOptions{
        DupMapKey:   cbor.DupMapKeyEnforcedAPF,   // performance hit, but safer
        IndefLength: cbor.IndefLengthForbidden,
        ...
    }
    safeDecMode = safeDecOptions.DecMode();
)

func Unmarshal(data []byte, dst interface{}) error {
    if data == nil {
        return nil
    }
    // return fastDecMode.Unmarshal(data, dst)
    return safeDecMode.Unmarshal(data, dst)
}

func NewDecoder(r io.Reader) *cbor.Decoder {
    // return fastDecMode.NewDecoder(r)
    return safeDecMode.NewDecoder(r)
}

Quick Start:

• https://github.com/fxamacker/cbor#quick-start

Decoding Options:

• https://github.com/fxamacker/cbor#decoding-options

Performance tip using fxamacker/cbor:

• Use Go structs instead of Go maps if possible. Take advantage of toarray and keyasint.

[cryptix]

I catchd an issue while switching back and forth between these and need to look deeper into how enforce cipher links.

err="failed retreive stored message: multiMessage: legacy decoding failed: cbor decode error [pos 132]: error reading tag; expected major type: 6, got: 0"

[cryptix]

@x448 if you have a moment, i was previously using StructToArray = true to default all encoding to that to reduce size overhead of lots repeating field names.

It seems like I need to put this in old the types now or am I missing something?

https://github.com/cryptoscope/ssb/commit/161d78ac1568014267156163550f0a6d4b8525a3#diff-f74db6ea5e1c6c7cacc62964178db98bR37

[x448]

@cryptix the "toarray" struct tag is the only to indicate all fields of the struct should encode to elements of CBOR array.

There's no option to do this for structs that don't have the "toarray" tag.

I think @fxamacker wanted to avoid having multiple ways for this to simplify struct metadata caching.

[x448]

I catchd an issue while switching back and forth between these and need to look deeper into how enforce cipher links.

err="failed retreive stored message: multiMessage: legacy decoding failed: cbor decode error [pos 132]: error reading tag; expected major type: 6, got: 0"

@cryptix it looks like maybe the encoder wasn't writing a CBOR tag (major type 6) and the decoder is expecting a CBOR tag.

If the encoder is fxamacker/cbor then look at EncOptions.TimeTag setting. It defaults to EncTagNone so time.Time values are not tagged by default (principle of smallest size and CBOR tags are optional).

Setting EncOptions.TimeTag = EncTagRequired will make time.Time value encode with a CBOR tag number which will make the CBOR data size larger.

If you think this will solve the problem, see:

• Quick Start Guide section "Creating and Using Encoding Modes" for example code.

  • Table of encoding options and default values.