Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Unused R in decryption #29

Open
TomMD opened this issue Oct 15, 2020 · 5 comments
Open

Unused R in decryption #29

TomMD opened this issue Oct 15, 2020 · 5 comments

Comments

@TomMD
Copy link

TomMD commented Oct 15, 2020

Looking here:

fpe/ff1/ff1.go

Line 484 in e6bfa0d

R := Y[:blockSize]

The comment suggests R should be used but it is entirely unused. (As reported by staticcheck here). Is this dead/delete-able or does it indicate a decryption error?

@anitgandhi
Copy link
Contributor

It's strange that staticcheck considers it unused, because it is consumed later on in the for loop with xored[offset+x] = R[x] ^ xored[offset+x].

Strictly speaking, I suppose it doesn't actually need to be a standalone variable, but I felt that for readability it makes sense to follow the algorithm definition/spec, Step 6.ii - Let R = PRF(P || Q). . It's then used in the xor step in 6.iii.

In the code, R is a "sub-slice" of Y and overlaps part of PQ. These all could have been separate, but as part of performance improvements in v1.1.0, they were changed to share a backing array.

@TomMD
Copy link
Author

TomMD commented Oct 15, 2020

I see, a false positive (a rather odd one too). I'm all in favor of the readability aspect of intermediate variables. Thanks for setting the record straight and for such a detailed response!

@TomMD TomMD closed this as completed Oct 15, 2020
@dominikh
Copy link

It's strange that staticcheck considers it unused, because it is consumed later on

Technically, it isn't. xored[offset+x] = R[x] ^ xored[offset+x] on line 554 makes use of the value of R set on line 534: R, err = c.prf(PQ). The definition of R on line 484 is never being read.

@TomMD
Copy link
Author

TomMD commented Oct 16, 2020

Reopening in light of @dominikh point - it appears not to be a false positive after all.

@TomMD TomMD reopened this Oct 16, 2020
@anitgandhi
Copy link
Contributor

Yeah indeed, thanks for pointing it out folks.
Unfortunately I no longer have write access to this repo so TBD on when this will get fixed...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants