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] ? StatusRequestV2Extension #246

Closed
VeNoMouS opened this issue Sep 28, 2023 · 7 comments
Closed

[BUG] ? StatusRequestV2Extension #246

VeNoMouS opened this issue Sep 28, 2023 · 7 comments
Labels
bug Unexpected behavior confirmed and should be fixed

Comments

@VeNoMouS
Copy link
Contributor

VeNoMouS commented Sep 28, 2023

Ran into a situation when using raw bytes, are we sure this was written correctly?

Shouldn't this

read more like...

// Write is a no-op for StatusRequestV2Extension. No data for this extension.
func (e *StatusRequestV2Extension) Write(b []byte) (int, error) {
	fullLen := len(b)
	extData := cryptobyte.String(b)
	// RFC 4366, Section 3.6
	var statusType uint8
	var ignored cryptobyte.String
	
	if !extData.ReadUint16LengthPrefixed(&ignored) ||
		!ignored.ReadUint8(&statusType) ||
		!ignored.ReadUint16LengthPrefixed(&ignored) ||
		!ignored.ReadUint16LengthPrefixed(&ignored) {
		//!extData.ReadUint16LengthPrefixed(&ignored) {
		return fullLen, errors.New("unable to read status request v2 extension data")
	}

	if statusType != statusV2TypeOCSP {
		return fullLen, errors.New("status request v2 extension statusType is not statusV2TypeOCSP(2)")
	}

	return fullLen, nil
}

extData becomes empty after the first ReadUint16LengthPrefixed... so you should traverse ignored after that...

Example

	fmt.Println()
	fmt.Println("Dumping b...")
	fmt.Println(b)
	fmt.Println("Dumping extData")
	fmt.Println(extData)
	fmt.Println()
	
	fmt.Println("Proccessing..")
	fmt.Println(ignored)
	extData.ReadUint16LengthPrefixed(&ignored)
	fmt.Println(ignored)
	ignored.ReadUint8(&statusType)
	fmt.Println(ignored)
	ignored.ReadUint16LengthPrefixed(&ignored)
	fmt.Println(ignored)
	ignored.ReadUint16LengthPrefixed(&ignored) 
	fmt.Println(ignored)
	fmt.Println()
Dumping b...
[0 7 2 0 4 0 0 0 0]
Dumping extData
[0 7 2 0 4 0 0 0 0]

Proccessing..
[]
[2 0 4 0 0 0 0]
[0 4 0 0 0 0]
[0 0 0 0]
[]
@gaukas
Copy link
Contributor

gaukas commented Sep 28, 2023

Good catch. I agree that this is a bug. Would you love to submit a pull request addressing this issue?

We did not implement enough testcases to cover all the extensions, and I basically reversed the Read() function for the Write(). So testcases would benefit this project as well.

@gaukas gaukas added the bug Unexpected behavior confirmed and should be fixed label Sep 28, 2023
@VeNoMouS
Copy link
Contributor Author

VeNoMouS commented Sep 28, 2023

Yea that's not a problem, unsure why ReadUint16LengthPrefixed reads the last element tho,,, as there should be one more read left... 07, 2, 04, 00, 00

I'll look into that (unless it treating it as \0)

@gaukas
Copy link
Contributor

gaukas commented Sep 28, 2023

why ReadUint16LengthPrefixed reads the last element

Can you be more specific on that? Like, which bytes are you referring to as the last element?

@VeNoMouS
Copy link
Contributor Author

VeNoMouS commented Sep 28, 2023

why ReadUint16LengthPrefixed reads the last element

Can you be more specific on that? Like, which bytes are you referring to as the last element?

with the last ignored.ReadUint16LengthPrefixed(&ignored) there should still be 0, 0 left in that array to read, but its empty, cryptobyte appears to do something with it... i'll run some tests

[2 0 4 0 0 0 0] <-- 8 
[0 4 0 0 0 0] <-- 16
[0 0 0 0] <-- 16
[] <--- missing

@gaukas
Copy link
Contributor

gaukas commented Sep 28, 2023

When you call ignored.ReadUint16LengthPrefixed(&ignored) when ignored == [0 0 0 0] you are reading (consuming) next two bytes from ignored as length (which is 0), then put ignored[0:length] == [] into &ignored. Basically that's where you lose the following bytes by overwriting ignored == [0 0] with [].

@VeNoMouS
Copy link
Contributor Author

ah right , my bad

@VeNoMouS
Copy link
Contributor Author

@gaukas PR sent #247

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected behavior confirmed and should be fixed
Projects
None yet
Development

No branches or pull requests

2 participants