Re: [openssl-project] to fully overlap or not to

[Bernd Edlinger]

On 03/08/18 12:06, Andy Polyakov wrote:
> 
>> Andy pointed out that my last e-mail was probably not clear enough.
>>
>> I want to drop the current partially overlapping checks
>> on the WRAP mode ciphers (which were ineffective anyways).
>>
>> And allow the following additional use case for any cipher
>>
>> unsigned char *in = buf + sizeof(buf) - X, *out = buf;
>> EVP_EncryptUpdate(ctx, out, , , sizeof(head));
>> out += outl;
>> EVP_EncryptUpdate(ctx, out, , in, X);
>> out += outl;
>> EVP_EncryptUpdate(ctx, out, , , sizeof(tail));
>> out += outl;
>>
>> with X > 75% sizeof(buf)
>> sizeof(head), sizeof(tail) not necessary multiple of block size
>>
>> And make the definition of allowed in-place partially overlapping
>> effectively be driven by the implementation requirements.
> 
> Nobody? OK. *Formal* objection to this is that you are making assumption
> that underlying implementation processes data in specific order. Note
> that it's not actually unreasonable assumption(*), yet in most general
> sense it's an assumption, and question rather is if you are in position
> to **formally** make it(**). One can argue that all our underlying
> implementations do that, process data in expected order that is, but it
> doesn't lift the question. One can even argue that suggested code worked
> in 1.0.1, yet it doesn't lift the question. So instead one simply aims
> for not making the assumption [or making least amount of assumptions].
> Yes, you can sense contradiction, because in-place processing also rests
> on an assumption. Yeah, world is not ideal and life is full of
> compromises. I mean there are pros and cons, and in-place is considered
> to bring more pros.
> 
> (*) And in some cases it's even 100% justified, most notably in CBC
> encrypt case, when each block is dependent on previous.
> 
> (**) Note that you can avoid the question by processing data strictly on
> per-block basis, so that you'll be in control of accessing data in
> specific order, not underlying implementation. But that's not what is
> suggested, right?

No.  The intention is to make the partially overlapping check follow
the underlying cipher's implementation needs, not the other way round.
And to document that the term "partially overlapping" is meant this way.


Bernd.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] to fully overlap or not to

[Bernd Edlinger]

Hi,

Andy pointed out that my last e-mail was probably not clear enough.

I want to drop the current partially overlapping checks
on the WRAP mode ciphers (which were ineffective anyways).

And allow the following additional use case for any cipher

unsigned char *in = buf + sizeof(buf) - X, *out = buf;
EVP_EncryptUpdate(ctx, out, , , sizeof(head));
out += outl;
EVP_EncryptUpdate(ctx, out, , in, X);
out += outl;
EVP_EncryptUpdate(ctx, out, , , sizeof(tail));
out += outl;

with X > 75% sizeof(buf)
sizeof(head), sizeof(tail) not necessary multiple of block size

And make the definition of allowed in-place partially overlapping
effectively be driven by the implementation requirements.


Bernd.

On 03/03/18 13:25, Bernd Edlinger wrote:
> On 03/01/18 10:59, Andy Polyakov wrote:
> I'd like to request more opinions on
> https://github.com/openssl/openssl/pull/5427. Key dispute question is
> whether or not following fragment should work
>
>  unsigned char *inp = buf, *out = buf;
>
>  for (i = 0; i < sizeof(buf); i++) {
>  EVP_EncryptUpdate(ctx, out, , inp++, 1);
> out += outl;
>  }

 This should work.

>>>
>>> It does only work, if you know that ctx->buf_len == 0
>>> before the loop is entered.
>>
>> It's naturally implied that ctx is used *consistently*. I mean it's not
>> like it's suggested that you can just use the above snippet in random
>> place. Application would have to adhere to above pattern all along, for
>> the life time of in-place processing "session". [I write "session" in
>> quotes, because there might be better term.]
>>
>>> It does not work with CFB1, CCM, XTS and WRAP modes.
>>
>> Yes, some modes are so to say "one-shot", i.e. you have to pass all the
>> data there is at once, no increments are allowed. But what does it have
>> to do with question at hand, question about in-place processing that is?
>> These two things are independent. So that question is rather should the
>> snippet work in cases when incremental processing is an option. Related
>> thing to recognize in the context is that *disputable* condition, the
>> one that triggered this discussion, is exercised only when
>> ctx->block_size is larger than 1, because then ctx->buf_len remains 0.
>> Note that ctx->block_size for CFB1, CCM and XTS is 1. As for WRAP, yeah,
>> it's special...
>>
>>> There is no access function to get the internal state of the cipher
>>> context.
>>
>> But there is notion of in-place processing "session".
>>
> 
> Well, I'd say, apparently the consensus is at least not to restrict
> what is currently recognized as "fully" overlapping.
> 
> All depends obviously on how "partially" overlapping is defined:
> a) by humans
> b) by OpenSSL
> 
> a)
> I think humans would normally say two objects are partially overlapping
> when they overlap, but do not fully overlap, in other words partially
> overlapping in and out do have some common bytes, but do not start at
> the same address, thus in != out.
> 
> b)
> What is currently recognized as partially overlapping in the evp_enc.c
> is a) but adds some I would say ad-hoc exceptions to accommodate one
> special use case with certain block ciphers.  Yet it does for instance
> artificially restrict use cases where other ciphers work just fine.
> 
> I think for instance of WRAP mode, where the primitive basically
> supports arbitrary overlapping (uses memmove) but we can no longer
> use that feature because of the partially overlapping check in the
> EVP cipher handling.
> 
> And the other use case, which I think is worth to mention is
> 
> unsigned char *inp = buf + sizeof(buf) - 1, *out = buf;
> for (i = 0; i      *inp = getc();
>      EVP_EncryptUpdate(ctx, out, , inp, 1);
>      out += outl;
> }
> 
> This could work for some cipher modes at least.
> 
> And I think our overall assumption here is the user knows perfectly
> well how the internal state of the ctx is at any time, otherwise
> EVP_EncryptUpdate would probably need to have an input parameter to
> tell how large the output buffer is in reality :)
> 
> So I think maybe we can just change the definition for b) to be
> 
> Partially overlapping is any kind of overlapping that is known to be
> not working in the evp cipher update function.
> It is dependent on the cipher mode, and dependent on the internal state
> of the cipher context.
> 
> It is often the case that partially overlapping means overlapping
> with in != out but most importantly the code in evp_enc.c is known
> to fail if it was allowed to continue after EVP_R_PARTIALLY_OVERLAPPING.
> 
> 
> Bernd.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] to fully overlap or not to

[Richard Levitte]

In message <39cb0562-d313-c2d7-8d84-58badaaaf...@openssl.org> on Wed, 28 Feb 
2018 18:09:38 +0100, Andy Polyakov  said:

appro> >>> I'd like to request more opinions on
appro> >>> https://github.com/openssl/openssl/pull/5427. Key dispute question is
appro> >>> whether or not following fragment should work
appro> >>>
appro> >>>   unsigned char *inp = buf, *out = buf;
appro> >>>
appro> >>>   for (i = 0; i < sizeof(buf); i++) {
appro> >>>   EVP_EncryptUpdate(ctx, out, , inp++, 1);
appro> >>>  out += outl;
appro> >>>   }
appro> >>
appro> >> This should work.
appro> > 
appro> > On second thought, perhaps not.
appro> 
appro> It seems that double-clarification is appropriate. As it stands now it
appro> *does* work. So question is rather if it should keep working [and if
appro> not, is it appropriate that stops working in minor release].

I'll side with that it should continue to work...  and most
definitely should NOT stop working in a minor release.

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] to fully overlap or not to

[Matt Caswell]

On 28/02/18 17:09, Andy Polyakov wrote:
 I'd like to request more opinions on
 https://github.com/openssl/openssl/pull/5427. Key dispute question is
 whether or not following fragment should work

   unsigned char *inp = buf, *out = buf;

   for (i = 0; i < sizeof(buf); i++) {
   EVP_EncryptUpdate(ctx, out, , inp++, 1);
out += outl;
   }
>>>
>>> This should work.
>>
>> On second thought, perhaps not.
> 
> It seems that double-clarification is appropriate. As it stands now it
> *does* work. So question is rather if it should keep working [and if
> not, is it appropriate that stops working in minor release].

It should continue to work and must not stop in a minor release.

Matt

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] to fully overlap or not to

[Andy Polyakov]

>>> I'd like to request more opinions on
>>> https://github.com/openssl/openssl/pull/5427. Key dispute question is
>>> whether or not following fragment should work
>>>
>>>   unsigned char *inp = buf, *out = buf;
>>>
>>>   for (i = 0; i < sizeof(buf); i++) {
>>>   EVP_EncryptUpdate(ctx, out, , inp++, 1);
>>> out += outl;
>>>   }
>>
>> This should work.
> 
> On second thought, perhaps not.

It seems that double-clarification is appropriate. As it stands now it
*does* work. So question is rather if it should keep working [and if
not, is it appropriate that stops working in minor release].

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] to fully overlap or not to

[Matt Caswell]

On 28/02/18 16:32, Viktor Dukhovni wrote:
> 
> 
>> On Feb 28, 2018, at 11:25 AM, Viktor Dukhovni  
>> wrote:
>>
>>> I'd like to request more opinions on
>>> https://github.com/openssl/openssl/pull/5427. Key dispute question is
>>> whether or not following fragment should work
>>>
>>>   unsigned char *inp = buf, *out = buf;
>>>
>>>   for (i = 0; i < sizeof(buf); i++) {
>>>   EVP_EncryptUpdate(ctx, out, , inp++, 1);
>>> out += outl;
>>>   }
>>
>> This should work.
> 
> On second thought, perhaps not.  A block cipher cannot provide output
> synchronously on byte boundaries.
> 

It should still work IMO. With a block size of 16 you should get 15
invocations of EVP_EncryptUpdate where outl is set to 0 afterwards,
followed by 1 invocation where outl is set to 16 afterwards. This should
still be the case for in-place encryption.

Matt
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

[openssl-project] to fully overlap or not to

[Andy Polyakov]

Hi,

I'd like to request more opinions on
https://github.com/openssl/openssl/pull/5427. Key dispute question is
whether or not following fragment should work

unsigned char *inp = buf, *out = buf;

for (i = 0; i < sizeof(buf); i++) {
EVP_EncryptUpdate(ctx, out, , inp++, 1);
out += outl;
}

[Just in case, corresponding corner case is effectively exercised in
evp_test.] Bernd argues that this is unreasonable, which I counter with
assertion that it doesn't matter how unreasonable this snippet is,
because since we support in-place processing, it's reasonable to expect
stream-cipher-like semantic as above to work even with block ciphers. As
it stands now, suggested modified code would accept in-place calls only
on block boundaries. Collateral question also is whether or not it would
be appropriate to make this kind of change in minor release.

Just in case, to give a bit of more general background. Benrd has shown
that current checks are not sufficient to catch all corner cases of
partially overlapping buffers. It was discussed that partially
overlapping is not same as fully overlapping, a.k.a. in-place
processing, with latter being in fact supported. And even though above
snippet can be formally viewed as a stretch, it's argued that it does
exhibit "legitimate intention" that deserves to be recognized and
supported. At least it was so far, in sense that it's not exactly a
coincidence that it currently works. [The fact that other corner cases
are not recognized as invalid is of course a bug, but there is no
contradiction, as fixing the bug doesn't have to mean that specific
corner case is no longer recognized.]

Thanks in advance.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project