#37350 [BC-Insight] `null` Is Not Unmarshalled Correctly Into json.RawMessage

Submitted on Dec 2nd 2024 at 17:25:43 UTC by @CertiK for Attackathon | Ethereum Protocol

Report ID: #37350
Report Type: Blockchain/DLT
Report severity: Insight
Target: https://github.com/ledgerwatch/erigon
Impacts:
- (Specifications) A bug in specifications with no direct impact on client implementations

Description

Brief/Intro

The response of null in the rpc package of Ethereum client Erigon ( https://github.com/erigontech/erigon ) could not be presented correctly due to the unmarshalling of the null in the CallContext.

Vulnerability Details

Affected Codebase:
https://github.com/erigontech/erigon/tree/v2.61.0-beta1

In the rpc package, the CallContext is used to perform a JSON-RPC call and unmarshal the returned result. However, the unmarshalling for the result of null is not correctly performed:

https://github.com/erigontech/erigon/blob/v2.61.0-beta1/rpc/client.go#L321

// CallContext performs a JSON-RPC call with the given arguments. If the context is
// canceled before the call has successfully returned, CallContext returns immediately.
//
// The result must be a pointer so that package json can unmarshal into it. You
// can also pass nil, in which case the result is ignored.
func (c *Client) CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error {
  if result != nil && reflect.TypeOf(result).Kind() != reflect.Ptr {
    return fmt.Errorf("call result parameter must be pointer or nil interface: %v", result)
  }
  msg, err := c.newMessage(method, args...)
  if err != nil {
    return err
  }
  op := &requestOp{ids: []json.RawMessage{msg.ID}, resp: make(chan *jsonrpcMessage, 1)}


  if c.isHTTP {
    err = c.sendHTTP(ctx, op, msg)
  } else {
    err = c.send(ctx, op, msg)
  }
  if err != nil {
    return err
  }


  // dispatch has accepted the request and will close the channel when it quits.
  switch resp, err := op.wait(ctx, c); {
  case err != nil:
    return err
  case resp.Error != nil:
    return resp.Error
  case len(resp.Result) == 0:
    return ErrNoResult
  default:
    return json.Unmarshal(resp.Result, &result)
  }
}

As mentioned in the geth(go-ethereum) PR: https://github.com/ethereum/go-ethereum/pull/26701

The function already checks that the result is either nil or a pointer type, so the extra reference operator is unnecessary. This actually causes a bug where nulls are not unmarshalled correctly into json.RawMessage.

It is worth noted a similar issue has been fixed in geth (go-ethereum) by fixing the result unmarshalling dependent of null:

https://github.com/ethereum/go-ethereum/pull/26723

func (c *Client) CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error {
   ...
  default:
    return json.Unmarshal(resp.Result, &result)
    if result == nil {
      return nil
    }
    return json.Unmarshal(resp.Result, result)
  }

Impact Details

null result could not be unmarshalled correctly in the json.RawMessage in the CallContext of rpc.

References

Proof of Concept

For simplicity, we can reuse and modify the test from geth (go-ethereum) (https://github.com/ethereum/go-ethereum/pull/26723) to verify the issue:

package rpc


import (
   "context"
   "encoding/json"
   "errors"
   "fmt"
   "math/rand"
   "net"
   "net/http"
   "net/http/httptest"
   "reflect"
   "strings"
   "sync"
   "testing"
   "time"


   "github.com/davecgh/go-spew/spew"
   "github.com/ledgerwatch/erigon-lib/common/dbg"
   "github.com/ledgerwatch/log/v3"
)




type nullTestService struct{}


func TestNullResponse(t *testing.T) {
   logger := log.New()
   server := newTestServer(logger)
   defer server.Stop()
   err := server.RegisterName("test", new(nullTestService))


   if err != nil {


      t.Fatal(err)
   }


   client := DialInProc(server, logger)
   defer client.Close()
   result := &jsonrpcMessage{}


   err = client.Call(&result.Result, "test_returnNull")
   if err != nil {
      t.Fatal(err)
   }


   if result.Result == nil {
      t.Fatal("Expected non-nil result")
   }


   if !reflect.DeepEqual(result.Result, json.RawMessage("null")) {
      t.Errorf("Expected null, got %s", result.Result)
   }
}

The output shows the Expected non-nil result error message when the result is nil.

=== RUN   TestNullResponse
[WARN] [12-02|12:17:37.643] Cannot register RPC callback [invalidRets1] - error must the last return value 
[WARN] [12-02|12:17:37.643] Cannot register RPC callback [invalidRets2] - error must the last return value 
[WARN] [12-02|12:17:37.643] Cannot register RPC callback [invalidRets3] - maximum 2 return values are allowed, got 3 
    client_test.go:140: Expected non-nil result
--- FAIL: TestNullResponse (0.00s)

FAIL