Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions core/vm/privacy/ringct.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import (
"encoding/binary"
"errors"
"fmt"
"github.com/XinFinOrg/XDPoSChain/common"
"math/big"

"github.com/XinFinOrg/XDPoSChain/common"

"github.com/XinFinOrg/XDPoSChain/crypto"
"github.com/XinFinOrg/XDPoSChain/log"
)
Expand All @@ -28,18 +29,18 @@ const (
pubkeyHybrid byte = 0x6 // y_bit + x coord + y coord
)

//The proof contains pretty much stuffs
//The proof contains pretty much stuffs
//Ring size rs: 1 byte => proof[0]
//num input: number of real inputs: 1 byte => proof[1]
//List of inputs/UTXO index typed uint64 => total size = rs * numInput * 8 = proof[0]*proof[1]*8
//List of key images: total size = numInput * 33 = proof[1] * 33
//number of output n: 1 byte
//List of output => n * 130 bytes
//transaction fee: uint256 => 32 byte
//ringCT proof size ctSize: uint16 => 2 byte
//ringCT proof: ctSize bytes
//bulletproofs: bp
// The proof contains pretty much stuffs
// The proof contains pretty much stuffs
// Ring size rs: 1 byte => proof[0]
// num input: number of real inputs: 1 byte => proof[1]
// List of inputs/UTXO index typed uint64 => total size = rs * numInput * 8 = proof[0]*proof[1]*8
// List of key images: total size = numInput * 33 = proof[1] * 33
// number of output n: 1 byte
// List of output => n * 130 bytes
// transaction fee: uint256 => 32 byte
// ringCT proof size ctSize: uint16 => 2 byte
// ringCT proof: ctSize bytes
// bulletproofs: bp
type PrivateSendVerifier struct {
proof []byte
//ringCT RingCT
Expand Down Expand Up @@ -174,7 +175,24 @@ func (r *RingSignature) Serialize() ([]byte, error) {
}

func computeSignatureSize(numRing int, ringSize int) int {
return 8 + 8 + 32 + 32 + numRing*ringSize*32 + numRing*ringSize*33 + numRing*33
const MaxInt = int(^uint(0) >> 1)

if numRing < 0 || ringSize < 0 {
return -1
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of numRing and ringSize must greater than zero:

	if numRing <= 0 || ringSize <= 0 {
		return -1
	}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be zero right? based on the logic

	numRing := r[offset : offset+8]
	offset += 8
	size := r[offset : offset+8]
	offset += 8

	size_uint := binary.BigEndian.Uint64(size)
	size_int := int(size_uint)
	sig.Size = size_int

	size_uint = binary.BigEndian.Uint64(numRing)
	size_int = int(size_uint)
	sig.NumRing = size_int

// Calculate each term separately and check for overflow
term1 := 8 + 8 + 32 + 32
term2 := numRing * ringSize * 32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could numRing * ringSize overflow? could numRing * ringSize * 32 overflow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it could, their types are both int

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is actually reading from signature

func Deserialize(r []byte) (*RingSignature, error) {
	if len(r) < 16 {
		return nil, errors.New("Failed to deserialize ring signature")
	}
	offset := 0
	sig := new(RingSignature)
	numRing := r[offset : offset+8]
	offset += 8
	size := r[offset : offset+8]
	offset += 8

	size_uint := binary.BigEndian.Uint64(size)
	size_int := int(size_uint)
	sig.Size = size_int

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if someone manipulate signature that intend to make it overflow, it's doable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should check multiplication overflow too

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the function computeSignatureSize should return uint, not int. The below function signature maybe better:

func computeSignatureSize(numRing uint64, ringSize uint64) uint64

When the compute result is overflow, we can return 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think -1 is better since the final check is compared length, this quarantine is definitely return error

	if len(r) != computeSignatureSize(sig.NumRing, sig.Size) {
		return nil, fmt.Errorf("incorrect ring size, len r: %d, sig.NumRing: %d sig.Size: %d", len(r), sig.NumRing, sig.Size)
	}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should check multiplication overflow too

@wgr523 I didn't get you

term3 := numRing * ringSize * 33
term4 := numRing * 33

if term2 < 0 || term3 < 0 || term4 < 0 || term2 > MaxInt-term1 || term3 > MaxInt-(term1+term2) || term4 > MaxInt-(term1+term2+term3) {
return -1
}

result := term1 + term2 + term3 + term4
return result
}

// deserializes the byteified signature into a RingSignature struct
Expand All @@ -198,7 +216,7 @@ func Deserialize(r []byte) (*RingSignature, error) {
sig.NumRing = size_int

if len(r) != computeSignatureSize(sig.NumRing, sig.Size) {
return nil, errors.New("incorrect ring size")
return nil, fmt.Errorf("incorrect ring size, len r: %d, sig.NumRing: %d sig.Size: %d", len(r), sig.NumRing, sig.Size)
}

m := r[offset : offset+32]
Expand Down Expand Up @@ -544,7 +562,7 @@ func Link(sig_a *RingSignature, sig_b *RingSignature) bool {
return false
}

//function returns(mutiple rings, private keys, message, error)
// function returns(mutiple rings, private keys, message, error)
func GenerateMultiRingParams(numRing int, ringSize int, s int) (rings []Ring, privkeys []*ecdsa.PrivateKey, m [32]byte, err error) {
for i := 0; i < numRing; i++ {
privkey, err := crypto.GenerateKey()
Expand Down
40 changes: 39 additions & 1 deletion core/vm/privacy/ringct_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package privacy

import (
"encoding/binary"
"fmt"
"testing"
)
)

func TestSign(t *testing.T) {
/*for i := 14; i < 15; i++ {
Expand Down Expand Up @@ -44,3 +45,40 @@ func TestSign(t *testing.T) {
}

}

func TestDeserialize(t *testing.T) {
numRing := 5
ringSize := 10
s := 5
rings, privkeys, m, err := GenerateMultiRingParams(numRing, ringSize, s)

ringSignature, err := Sign(m, rings, privkeys, s)
if err != nil {
t.Error("Failed to create Ring signature")
}

// A normal signature.
sig, err := ringSignature.Serialize()
if err != nil {
t.Error("Failed to Serialize input Ring signature")
}

// Modify the serialized signature s.t.
// the new signature passes the length check
// but triggers buffer overflow in Deserialize().
// ringSize: 10 -> 56759212534490939
// len(sig): 3495 -> 3804
// 80 + 5 * (56759212534490939*65 + 33) = 18446744073709551616 + 3804
bs := make([]byte, 8)
binary.BigEndian.PutUint64(bs, 56759212534490939)
for i := 0; i < 8; i++ {
sig[i+8] = bs[i]
}
tail := make([]byte, 3804-len(sig))
sig = append(sig, tail...)

_, err = Deserialize(sig)
if err != nil {
t.Error("Failed to Serialize input Ring signature")
}
}