-
Notifications
You must be signed in to change notification settings - Fork 1
Verification bugfixes #6
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
Conversation
…rification Fix TSA name formatting Better Base64 whitespace handling
…ding Normalize ECDSA signatures to IEEE P1363 format (raw R||S) expected by Web Crypto API. Some XML signatures have R and S components padded with leading zeros (e.g., 66 bytes instead of 64 for P-256), causing verification failures.
Greptile Summary
Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant verifySignature
participant verifyTimestamp
participant parseTimestamp
participant checkCertificateRevocation
participant verifySignedInfo
participant verifyChecksums
User->>verifySignature: "Verify signature with options"
alt has signatureTimestamp and verifyTimestamps !== false
verifySignature->>verifyTimestamp: "Verify timestamp"
verifyTimestamp->>parseTimestamp: "Parse timestamp token"
parseTimestamp-->>verifyTimestamp: "Return timestamp info"
verifyTimestamp-->>verifySignature: "Return verification result"
verifySignature->>verifySignature: "Update trustedSigningTime if valid"
end
verifySignature->>verifyCertificate: "Verify certificate with trusted time"
verifyCertificate-->>verifySignature: "Return certificate result"
alt checkRevocation !== false and certificate valid
verifySignature->>checkCertificateRevocation: "Check revocation status"
checkCertificateRevocation-->>verifySignature: "Return revocation result"
end
alt verifyChecksums !== false
verifySignature->>verifyChecksums: "Verify file checksums"
verifyChecksums-->>verifySignature: "Return checksum results"
end
alt verifySignatures !== false and has required components
verifySignature->>verifySignedInfo: "Verify XML signature"
verifySignedInfo-->>verifySignature: "Return signature verification"
end
verifySignature->>verifySignature: "Determine overall validity"
verifySignature-->>User: "Return complete verification result"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, 2 comments
| // Skip sequence length (may be 1 or 2 bytes) | ||
| const seqLen = derSignature[offset++]; | ||
| if (seqLen & 0x80) { | ||
| offset += seqLen & 0x7f; // Skip length bytes | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: DER length parsing may fail for long form lengths > 127 bytes. Should this handle multi-byte length encoding properly for large signatures?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/verification.ts
Line: 432:436
Comment:
**logic:** DER length parsing may fail for long form lengths > 127 bytes. Should this handle multi-byte length encoding properly for large signatures?
How can I resolve this? If you propose a fix, please make it concise.| // Component too large - take the last 'size' bytes | ||
| return stripped.slice(stripped.length - size); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Truncating oversized components could create invalid signatures - consider throwing error instead
| // Component too large - take the last 'size' bytes | |
| return stripped.slice(stripped.length - size); | |
| } | |
| } else { | |
| // Component too large - this indicates corrupted or invalid data | |
| throw new Error(`ECDSA component too large: expected ${size} bytes, got ${stripped.length} bytes`); | |
| } |
Is silently truncating the component the intended behavior for oversized R/S values?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/verification.ts
Line: 413:415
Comment:
**logic:** Truncating oversized components could create invalid signatures - consider throwing error instead
```suggestion
} else {
// Component too large - this indicates corrupted or invalid data
throw new Error(`ECDSA component too large: expected ${size} bytes, got ${stripped.length} bytes`);
}
```
Is silently truncating the component the intended behavior for oversized R/S values?
How can I resolve this? If you propose a fix, please make it concise.
coversSignature: falseissue[object Object]instead of readable DN string likeCN=..., O=..., C=...atoberrors when decoding base64 strings containing whitespace from XML