Skip to content

Fix: detect and recover from false positive finder patterns#85

Open
bashhack wants to merge 1 commit intomakiuchi-d:masterfrom
bashhack:fix/false-positive-finder-recovery
Open

Fix: detect and recover from false positive finder patterns#85
bashhack wants to merge 1 commit intomakiuchi-d:masterfrom
bashhack:fix/false-positive-finder-recovery

Conversation

@bashhack
Copy link
Copy Markdown
Contributor

Summary

  • Add timing pattern cross-validation in ProcessFinderPatternInfo, this verifies alternating black and white pattern exists between finder pattern centers, rejecting false positives from QR data
  • Add version consistency check in computeDimension that both sides of the finder pattern triple must imply the same QR version +/- 1
  • Add FindExhaustive() method that scans the full image without early termination, collecting all finder pattern candidates
  • On ProcessFinderPatternInfo failure, retry with exhaustive scan so SelectBestPatterns has more candidates to pick the geometrically correct triple

Context

For ~5% of randomly generated TOTP secrets, QR data accidentally contains patterns matching the 1:1:3:1:1 finder pattern ratio. The scanner accepts these as a third finder pattern and stops early, never reaching the real one. The resulting wrong triple produces an incorrect dimension and a failed decode.

The timing and dimension checks now detect these false positives. The exhaustive retry recovers by rescanning the full image to find the real patterns. Together they reduce the observed failure rate from ~5% to ~0.1% on randomly generated TOTP QR codes at 200-400px sizes.f

Testing

[x] All qrcode/detector and qrcode tests pass
[x] Synthetic version 7 test updated with timing patterns
[x] makeTimingPattern test helper added for future test construction

@bashhack
Copy link
Copy Markdown
Contributor Author

@makiuchi-d wanted to give some context on the cumulative changes in this PR and #84 so you can ground the impact, context and opportunity.

I ran into the suite of changes (hopefully - improvements!) in the midst of working with TOTP codes, specifically some stress testing around TOTP/QR decoding.

At any rate - to measure the impact of the changes, I tested with randomly generated TOTP secrets encoded as QR codes using pquerna/otp (see: https://github.com/pquerna/otp) for generation and gozxing for decode. Single-threaded, 5000 random secrets per size.


Before any fixes from the PRs I've posted:

Size Failure rate
200x200 ~5.0%
300x300 ~5.0%
400x400 ~5.0%

After the PRs really #84 and #85, with #84 being the biggest single factor:

Size Failure rate
200x200 ~0.14%
300x300 ~0.10%
400x400 ~0.06%

⚠️ NOTE: These numbers include the effect of #82 , #83 , #84 as well - so the full improvement I think ultimately is the sum total of the changes.

The remaining ~0.1% failures break down for me as:

  • ChecksumException - correct finder patterns, correct version, but data has too many bit errors for Reed-Solomon. This I think must do with some sort of binarization/grid sampling fidelity issue, but I'm fairly confident it's not a finder pattern problem.

  • Rare FormatException - exhaustive scan found more candidates but all available triples have similar distortion scores


Root cause

For ~5% of randomly generated TOTP secrets, QR data accidentally contains patterns matching the 1:1:3:1:1 finder pattern ratio. The scanner accepts a false positive as the third finder pattern and HaveMultiplyConfirmedCenters() returns true (module sizes match), so the scanner stops early - never reaching the real third pattern. The wrong triple produces an incorrect dimension and a failed decode.

Example from a version 8 QR in a 400x400 image:
Real finder pattern positions were:
Top-Left: (32, 32) found
Top-Right: (368, 368) found
Bottom-Left: (32, 368) NOT FOUND - scanner stopped at y=292
False positive: (368, 292) INCORRECTLY FOUND - QR data mimics 1:1:3:1:1 ratio


The test I used for reference is provided here:

 func TestSingleThreadReliability(t *testing.T) {
        sizes := []int{200, 300, 400}
        runs := 5000
        for _, size := range sizes {
                failures := 0
                for i := 0; i < runs; i++ {
                        key, err := totp.Generate(totp.GenerateOpts{
                                Issuer:      "TestApp",
                                AccountName: "test@example.com",
                        })
                        if err != nil {
                                t.Fatalf("generate: %v", err)
                        }
                        rawImg, err := key.Image(size, size)
                        if err != nil {
                                t.Fatalf("image: %v", err)
                        }
                        var buf bytes.Buffer
                        if err := png.Encode(&buf, rawImg); err != nil {
                                t.Fatalf("encode: %v", err)
                        }
                        img, err := png.Decode(&buf)
                        if err != nil {
                                t.Fatalf("decode png: %v", err)
                        }
                        if _, err := qrcode.DecodeQRCodeFromImage(img); err != nil {
                                failures++
                        }
                }
                fmt.Printf("  %dx%d: %d/%d failed (%.2f%%)\n", size, size, failures, runs,
  float64(failures)*100/float64(runs))
        }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant