Cyphrme / Coze

Coze is a cryptographic JSON messaging specification.
https://cyphr.me/coze
BSD 3-Clause "New" or "Revised" License
108 stars 3 forks source link

MapItem is unsafe, and MapSlice does not have a well-defined order #10

Closed peterbourgon closed 1 year ago

peterbourgon commented 1 year ago

indexCounter is accessed without synchronization, which produces data races that violate the memory model. As a proof of concept, build and run the following program with the -race flag.

package main

import (
    "encoding/json"
    "sync"

    "github.com/cyphrme/coze"
)

func main() {
    n := 10
    var wg sync.WaitGroup
    for i := 0; i < n; i++ {
        wg.Add(1)
        go func() {
            defer wg.Done()
            var item coze.MapItem
            json.Unmarshal([]byte(`{}`), &item)
        }()
    }
    wg.Wait()
}

You'll see output like the following.

==================
WARNING: DATA RACE
Read at 0x00010041e838 by goroutine 13:
  github.com/cyphrme/coze.nextIndex()
      .../pkg/mod/github.com/cyphrme/coze@v0.0.3/mapslice.go:36 +0xf0
  github.com/cyphrme/coze.(*MapItem).UnmarshalJSON()
      .../pkg/mod/github.com/cyphrme/coze@v0.0.3/mapslice.go:118 +0x11c
  encoding/json.(*decodeState).object()
      ...

Previous write at 0x00010041e838 by goroutine 12:
  github.com/cyphrme/coze.nextIndex()
      .../pkg/mod/github.com/cyphrme/coze@v0.0.3/mapslice.go:36 +0x108
  github.com/cyphrme/coze.(*MapItem).UnmarshalJSON()
      .../pkg/mod/github.com/cyphrme/coze@v0.0.3/mapslice.go:118 +0x11c
  encoding/json.(*decodeState).object()
      ...
peterbourgon commented 11 months ago

https://go.dev/play/p/yBrTPLzHQWN

If you designate a payload as JSON, then step 2 is perfectly valid, and can occur anywhere between sender and receiver.

edit: I'm not trying to be argumentative, or anything like that. I'm only trying to point out an invalid assumption in the project. At this point I think I've done about as much as I can do to point out the issue, so I'll bow out, do with my comments what you will.

zamicol commented 11 months ago

Coze signs strings. That string has an invalid signature and that example is working as specified. It's doing exactly what it should do.

What invalid assumption? That Coze signs and verifies strings?

peterbourgon commented 11 months ago

What invalid assumption? That Coze signs and verifies strings?

No, that JSON serialization (as defined by the spec) produces "strings" (byte sequences) that can be signed/verified in the first place.

edit: You seem to be assuming that the specific bytes that MarshalJSON produces will not be modified between sender and receiver. That's simply not true, as a statement of fact, and regardless if those bytes are encoded as UTF-8 or otherwise.