183146 - buffer length bug in NSC_EncryptUpdate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

In netscape.public.mozilla.crypto, [e-mail] reported 
an apparent bug in NSC_EncryptUpdate.  The code now reads (near line 757)

    /* encrypt the current padded data */
    rv = (*context->update)(context->cipherInfo,pEncryptedPart, 
    &outlen,context->blockSize,context->padBuf,context->blockSize);
    if (rv != SECSuccess) return CKR_DEVICE_ERROR;
    pEncryptedPart += padoutlen;
    maxout -= padoutlen;

pingzhenyu suggests that the third argument to context->update should be
&padoutlen rather than &outlen, since the value of padoutlen, not outlen,
is subsequently added to pEncryptedPart and subtracted from maxout.

But in looking at this code further, I think this problem is only the tip
of the iceberg.  The block of code that begins near line 740 with the test

    /* do padding */
    if (context->doPad) {

which handles saving of partial blocks from one update call to the next
clearly needs to be done for ALL block ciphers.  But it is presently being
done only for those block cipher mechanisms that feature PKCS#7 padding.  

For example, context->doPad is set to TRUE for CKM_DES_CBC_PAD but not
for CKM_DES_CBC, yet the handling of partial blocks is needed for both.

I think softoken has been getting away with this because the higher layers
tend not to do encryption updates with partial blocks.  SSL/TLS, for example,
does its own padding for partial blocks, and so always calls update with 
full blocks.  Perhaps the PKCS#7 (SMIME) code behaves similarly.  But the
token's behavior appears to be wrong for partial block updates on block 
ciphers.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P1.  IMO, we should fix this ASAP.

I think we may need separate flags for doing PKCS#7 padding versus handling
partial blocks. Padding is only an issue in NSC_EncryptFinal.  Partial block
handling is needed in both EncryptUpdate and EncryptFinal.

Comment 2

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 3

[Robert Relyea]

OK, Nelson points out two issues, one from the news group, and one from his
inspection.

I haven't looked at the first problem, but the second issue is a
misunderstanding of the semantics of various PKCS #11 mechanisms. Only the _PAD
functions allow partial block encryption. The non-_PAD functions require the
higher level code to pass full blocks down anyway. The code isn't accidentally
working because the higher level "happens" to pass full blocks, the code is
working because the upper level code is "required" to pass full blocks. That is
why the upper level code knows about block size of the underlying algorithms.

Anyway I'll look at the first problem. Since this does not appear to be a crash,
and the P1 severity was set under the assumption that the entire infrasture was
incorrect, I'm setting the priority down again.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

In PKCS#11 v2.11r1, the section that defines C_Encrypt contains this paragraph:

> For some encryption mechanisms, the input plaintext data has certain length 
> constraints (either because the mechanism can only encrypt relatively short 
> pieces of plaintext, or because the mechanism’s input data must consist of 
> an integral number of blocks). If these constraints are not satisfied, then 
> C_Encrypt will fail with return code CKR_DATA_LEN_RANGE.

Each of the unpadded block cipher mechanism definitions has a table that 
clearly shows that, when used with the function C_Encrypt, the input must 
be a multiple of the block size.  These tables are all silent on the input 
lengths usable with C_EncryptUpdate.  

C_Encrypt only encrypts single part data, so it is reasonable to require 
that the input of the single part be a multiple of the block size in the 
absense of padding.

When encrypting in pieces, it likewise seems entirely reasonable to require
that the TOTAL amount of input sent to C_EncryptUpdate (the SUM of the 
amounts passed in individual calls to C_EncryptUpdate) be a multiple of 
the block size, but it does not seem reasonable to require that each 
individual update be a multiple of the block size.  

I can find no text in the PKCS spec that speaks to allowable input lengths
for C_EncryptUpdate (or C_DecryptUpdate) when used with unpadded block ciphers.  
Section 11.2, "Convention for functions returning output in a variable length 
buffer", gives an example that uses a padded block cipher.  All the pieces
of sample code that demonstrate the use of any Encrypt or Decrypt functions
use a padded block cipher.

In PKCS#11 v2.11r1, the section that defines C_EncryptUpdate does not contain 
any wording about input length limits similar to the wording for C_Encrypt.
It only refers to section 11.2.

The section that defines C_EncryptFinal has this paragraph:

> For some multi-part encryption mechanisms, the input plaintext data has 
> certain length constraints, because the mechanism’s input data must consist 
> of an integral number of blocks. If these constraints are not satisfied, 
> then C_EncryptFinal will fail with return code CKR_DATA_LEN_RANGE.

This seems to strongly imply that the total length input to C_EncryptUpdate
prior to calling C_EncryptFinal must meet the length requirements of the 
mechanism.  

I also observe that each of the unpadded block cipher mechanisms is defined
to do null padding of input that is not a block size multiple, but this is 
defined for C_WrapKey only, not for C_Encrypt.  

So, I conclude that PKCS#11 v2.11r1 does not require the intput length for
an individual call to C_EncryptUpdate to be a multiple of the input blocksize
for any non-padded symmetric block cipher.  

Since softoken already contains all the necessary code to provide for partial 
block updates, I see no reason to disallow its use for unpadded block ciphers.

Comment 5

[Robert Relyea]

The bufferlength bug has been fixed and checked into both the tip and 3.7.

The other issue of what is the correct PKCS #11 semantic is a separate issue and
falls under the category of "Making softoken more PKCS #11 complient", and
should be tracked as a separate issue.

closing this bug now.