Skip to content
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.

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

bug: DecodeRLP can panic #11053

Closed
arajasek opened this issue Jul 5, 2023 · 0 comments · Fixed by #11079
Closed

bug: DecodeRLP can panic #11053

arajasek opened this issue Jul 5, 2023 · 0 comments · Fixed by #11079
Assignees
Labels

Comments

@arajasek
Copy link
Contributor

arajasek commented Jul 5, 2023

Report

We received the following report on ImmuneFi:

Bug Description
Lotus client node panics when it tries to decode malformed RLP input.
func decodeRLP(data []byte) (res interface{}, consumed int, err error) {
	if len(data) == 0 {
		return data, 0, xerrors.Errorf("invalid rlp data: data cannot be empty")
	}
	if data[0] >= 0xf8 {
		...
	} else if data[0] >= 0xc0 {
		...
	} else if data[0] >= 0xb8 {
		strLenInBytes := int(data[0]) - 0xb7
#1		strLen, err := decodeLength(data[1:], strLenInBytes)
		if err != nil {
			return nil, 0, err
		}
#2		totalLen := 1 + strLenInBytes + strLen
#3		if totalLen > len(data) {
			return nil, 0, xerrors.Errorf("invalid rlp data: out of bound while parsing string")
		}
#4		return data[1+strLenInBytes : totalLen], totalLen, nil
	} else if data[0] >= 0x80 {
	  ...
	}
	return []byte{data[0]}, 1, nil
}
line #1 - strLen can be negative (its type is ‘int’)
line #2 - totalLen can be negative
line #3 - checks will be bypassed if totalLen < 0
line #4 - out of bounds panic

Code in question:

func decodeRLP(data []byte) (res interface{}, consumed int, err error) {

Assessment

The core of the report appears to be valid, confirmed by adding the following test case to a Lotus itest:

	txPanic, err := hex.DecodeString("02bfffffffffffffff0041424344")
	require.NoError(t, err)
	_, err = client.EthSendRawTransaction(ctx, txPanic)
	//require.NoError(t, err)

Impact

As far as I can tell, the impact is relatively low, because:

  • The code in question can only be triggered by calls to EthSendRawTransaction
  • The EthSendRawTransaction method (and EthAPI in general) is disabled by default
  • Even if triggered on a public node exposing the EthAPI, go-jsonrpc traps all panics, and so shouldn't lead to any issues

This has been confirmed by @fridrik01 and @Stebalien

Next steps

  • Check for similar problems in other parts of the FEVM-related code, especially anything on the critical path
  • Correctly (panic-lessly) decode RLP here, and anywhere else
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants