Closed Bug 1251233 Opened 5 years ago Closed 5 years ago

[CID 1354262][CID 1354261] out-of-bound access in ChaCha20+Poly1305

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED INVALID
Tracking Status
firefox47 --- affected

People

(Reporter: franziskus, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1354262, CID 1354261)

In sftk_ChaCha20Poly1305_Encrypt in pkcs11c.c we set ad = ctx->ad if there's no ctx->adOverflow. In this case ad is unsigned char[16]. But in poly1305_blocks in poly1305-donna-x64-sse2-incremental-source.c tries to access ad[56].

>     const unsigned char *ad = ctx->adOverflow;
> 
>    if (ad == NULL) {
>        ad = ctx->ad;
>    }
> 
>    return ChaCha20Poly1305_Seal(&ctx->freeblCtx, output, outputLen,
>                                 maxOutputLen, input, inputLen, ctx->nonce,
>                                 sizeof(ctx->nonce), ad, ctx->adLen);

I don't think this is particularly dangerous, but let's investigate first.
The problem exists in sftk_ChaCha20Poly1305_Decrypt
Summary: [CID 1354262] out-of-bound access in ChaCha20+Poly1305 → [CID 1354262][CID 1354261] out-of-bound access in ChaCha20+Poly1305
Whiteboard: CID 1354262 → CID 1354262, CID 1354261
Resolving as invalid after Franziskus and I talked about it and took a deep look.

Coverity is confused because it doesn't know that Poly1305Update() collects data until we have a 64-byte block and then passes that to poly1305_blocks(). The variable |want| passed to poly1305_blocks() will always contain a multiple of 64, because (bytes >= 64) and want = (bytes & ~63).

Apart from that, additional data will always go into the first block. If it's longer than 32 bytes then the overflow will go into subsequent 64-byte blocks coming after the first one.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Group: crypto-core-security
You need to log in before you can comment on or make changes to this bug.