[openssl-dev] [openssl.org #4343] master: EC_KEY_priv2buf (): check parameter sanity

[J Mohan Rao Arisankala via RT]

Hi,

I have PR https://github.com/openssl/openssl/pull/739 with the below
changes, please have a look.

- In EC_KEY_priv2buf(), check for pbuf sanity.
- If invoked with NULL, gracefully returns the key length.

Thanks,
Mohan

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4343
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[Rich Salz via RT]

commit acae59b pushed, thanks!
--
Rich Salz, OpenSSL dev team; rs...@openssl.org

[Stephen Henson via RT]

On Wed Feb 24 12:07:05 2016, mo...@computer.org wrote:
> Hi,
>
> I have PR https://github.com/openssl/openssl/pull/739 with the below
> changes, please have a look.
>
> - In EC_KEY_priv2buf(), check for pbuf sanity.
> - If invoked with NULL, gracefully returns the key length.
>

If you're doing this you're probably using the wrong API. EC_KEY_priv2buf()
allocates a buffer and returns its length. If you just want the length and/or
want to allocate a buffer yourself you should be using EV_KEY_priv2oct()
instead.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 04:50:27PM +0000, Stephen Henson via RT wrote:

> > I have PR https://github.com/openssl/openssl/pull/739 with the below
> > changes, please have a look.
> >
> > - In EC_KEY_priv2buf(), check for pbuf sanity.
> > - If invoked with NULL, gracefully returns the key length.
> >
>
> If you're doing this you're probably using the wrong API. EC_KEY_priv2buf()
> allocates a buffer and returns its length. If you just want the length and/or
> want to allocate a buffer yourself you should be using EV_KEY_priv2oct()
> instead.

I'd like to propose a policy of no bug fixes to undocumented public
interfaces. If the interface is useful enough to fix, it has to be
documented. Anyone care to produce manpages for EC_KEY_priv2buf or
EC_KEY_priv2oct?

--
Viktor.

[Jeffrey Walton]

>> > I have PR https://github.com/openssl/openssl/pull/739 with the below
>> > changes, please have a look.
>> >
>> > - In EC_KEY_priv2buf(), check for pbuf sanity.
>> > - If invoked with NULL, gracefully returns the key length.

> ...

> I'd like to propose a policy of no bug fixes to undocumented public
> interfaces. If the interface is useful enough to fix, it has to be
> documented. Anyone care to produce manpages for EC_KEY_priv2buf or
> EC_KEY_priv2oct?
>

Correct me if I am wrong... API's that start with capitol letters are
public. Private interfaces use lowercase letters.
Documented/undocumented does not really factor things.

If OpenSSL wants to make it private so that its should not be called
and it won't be maintained, then the symbol should be changed to
ec_key_priv2oct.

Jeff

[Salz, Rich]

> I'd like to propose a policy of no bug fixes to undocumented public
> interfaces.

That seems extreme, given how much of the API is undocumented and how much external stuff depends on private things. I understand the goal. I just want to make sure you've thought about the proposal. (And heck I might even agree with it)

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 12:10:09PM -0500, Jeffrey Walton wrote:

> > I'd like to propose a policy of no bug fixes to undocumented public

> > interfaces. If the interface is useful enough to fix, it has to be
> > documented. Anyone care to produce manpages for EC_KEY_priv2buf or
> > EC_KEY_priv2oct?
> >
> Correct me if I am wrong... API's that start with capitol letters are
> public. Private interfaces use lowercase letters.
> Documented/undocumented does not really factor things.

You're wrong. Once OpenSSL's past sins are remediated, public
interfaces are precisely those that are documented. For now, public
interfaces are either macros or functions and global variables
those whose symbols are exported by the libssl and libcrypto shared
libraries. This is not a good place to be.

> If OpenSSL wants to make it private so that its should not be called
> and it won't be maintained, then the symbol should be changed to
> ec_key_priv2oct.

Undocumented functions have no public "contract", and should not
be used. Sadly we're not quite there yet, but the way to get there
is at least always update the documentation of any functions that
are updated. We also have to document functions that are not
updated, but that does not change the wisdom of the proposed policy.

One reason to update the documentation, is that one will often find
that documenting a function will make one think harder about how it
is really supposed to behave, potentially improving the code and/or
avoiding mistakes.

--
Viktor.

[Jeffrey Walton]

>> > I'd like to propose a policy of no bug fixes to undocumented public
>> > interfaces. If the interface is useful enough to fix, it has to be
>> > documented. Anyone care to produce manpages for EC_KEY_priv2buf or
>> > EC_KEY_priv2oct?
>> >
>> Correct me if I am wrong... API's that start with capitol letters are
>> public. Private interfaces use lowercase letters.
>> Documented/undocumented does not really factor things.
>
> You're wrong. Once OpenSSL's past sins are remediated, public
> interfaces are precisely those that are documented. For now, public
> interfaces are either macros or functions and global variables
> those whose symbols are exported by the libssl and libcrypto shared
> libraries. This is not a good place to be.
>

Thanks. Is that documented anywhere? (I'm still providing the old
guidance, so it would be nice to cite the change and the documented
policy).

Jeff

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 05:10:42PM +0000, Salz, Rich wrote:

> > I'd like to propose a policy of no bug fixes to undocumented public
> > interfaces.
>

> That seems extreme, given how much of the API is undocumented and how much
> external stuff depends on private things.

Not at all. You're well aware, for example, that the bulk of my
changes for the upcoming security release are documentation; the
code changes were easy enough.

> I understand the goal. I just want to make sure you've thought about the
> proposal. (And heck I might even agree with it)

I've thought about it a great deal over the years. This policy
has proved its worth over and over in other contexts and recently
in OpenSSL as well. The code changes for 1.0.2g would not have
been as complete, had I not reviewed and updated the documentation.

--
Viktor.

[Salz, Rich]

As just about the only team member who trolls through RT and closes things with any quantity, I am not sure that I agree that fixing a bug requires documentation if the API isn't already documented.

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 05:29:26PM +0000, Salz, Rich wrote:

> As just about the only team member who trolls through RT and closes things
> with any quantity, I am not sure that I agree that fixing a bug requires
> documentation if the API isn't already documented.

Focus on fixing bugs in documented interfaces first, and even then review
the documentation, to make sure the fix comports with the documentation and
that the latter is not stale.

Once all the bugs in documented interfaces are fixed, I'm afraid
we really must start documenting the code we're updating. Otherwise,
there's no way to know we're doing it right, and we're digging the
hole deeper. New code needs to be documented as it is written,
old code also, but at a slower pace.

--
Viktor.

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 05:29:26PM +0000, Salz, Rich wrote:

> As just about the only team member who trolls through RT and closes things
> with any quantity, I am not sure that I agree that fixing a bug requires
> documentation if the API isn't already documented.

We should also get the word out that contributed patches (RT or
Github) without documentation will take much longer to get adopted
(will require someone else to find the time to create the
documentation).

Priority will go to patches with sufficiently complete documentation.

[Jeffrey Walton]

On Fri, Feb 26, 2016 at 12:29 PM, Salz, Rich <rs...@akamai.com> wrote:
> As just about the only team member who trolls through RT and closes things with any quantity, I am not sure that I agree that fixing a bug requires documentation if the API isn't already documented.

+1. Concepts seem to be cross-pollinating.

It seems like (to me) the the most direct way to mark a function as
private is to add a comment in the source code stating such. That will
avoid pain points for public but undocumented functions.

it also seems like (to me) that tying bug fixes to documentation is a bad idea.

Jeff

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 12:37:22PM -0500, Jeffrey Walton wrote:

> It seems like (to me) the the most direct way to mark a function as
> private is to add a comment in the source code stating such.

Nonsense. Source code is not API documentation, it is an
implementation, not an interface contract.

> That will avoid pain points for public but undocumented functions.

There's must (as soon as we can get there) be no such thing as a
"public, but undocumented" function.

> it also seems like (to me) that tying bug fixes to documentation is a bad idea.

Bug fixes to undocumented functions will be buggy, and the
documentation will never happen. We need to improve code quality,
a good part of that is having documentation.

--
Viktor.

[Jeffrey Walton]

On Fri, Feb 26, 2016 at 12:42 PM, Viktor Dukhovni
<openss...@dukhovni.org> wrote:
> On Fri, Feb 26, 2016 at 12:37:22PM -0500, Jeffrey Walton wrote:
>
>> It seems like (to me) the the most direct way to mark a function as
>> private is to add a comment in the source code stating such.
>
> Nonsense. Source code is not API documentation, it is an
> implementation, not an interface contract.

I'm not sure I'd consider it nonsense.

Studying source code on occasion is simply par for the course when
working with open source libraries. I'm not sure how that no longer
applies. That won't chnage by fiat.

In my naiveness, if you want something to be private, then you don't
expose it to the outside world.

It also seems like if a function leaks into a public header, then you
state that its private and it should not be called. What's the
aversion to a clearly and succinctly stating something?

>> ...

>> it also seems like (to me) that tying bug fixes to documentation is a bad idea.
>
> Bug fixes to undocumented functions will be buggy, and the
> documentation will never happen. We need to improve code quality,
> a good part of that is having documentation.

It sounds like you are trying to get half pregnant. If the code is
buggy, then it should be fixed or removed. There's no middle ground.

Jeff

[Viktor Dukhovni]

On Fri, Feb 26, 2016 at 12:50:24PM -0500, Jeffrey Walton wrote:

> > Nonsense. Source code is not API documentation, it is an
> > implementation, not an interface contract.
>
> I'm not sure I'd consider it nonsense.

Comments in source code are not documentation, they explain the
internals of the implementation, not the contract. Users of a
library need to depend on documented semantics, not implementation
artefacts.

> Studying source code on occasion is simply par for the course when
> working with open source libraries.

Here, by "open source" you mean poorly maintained. I'd like OpenSSL
to leave that legacy behind. Not all open source software is poorly
maintained and under-documented.

> In my naiveness, if you want something to be private, then you don't
> expose it to the outside world.

That too, but not documenting an interface should be sufficient to
dissuade users from relying on it. Sadly, important parts of
OpenSSL are not yet documented, and I am proposing we become more
aggressive about fixing that.

One way forward is to ensure that changes must come with documentation,
created if missing, updated if the interface is extended, or changed
incompatibly.

> It also seems like if a function leaks into a public header, then you
> state that its private and it should not be called. What's the
> aversion to a clearly and succinctly stating something?

All I'm saying is that documentation is not optional. Neither source
code nor header files are documentation.

> > Bug fixes to undocumented functions will be buggy, and the
> > documentation will never happen. We need to improve code quality,
> > a good part of that is having documentation.
>

> If the code is
> buggy, then it should be fixed or removed. There's no middle ground.

It must be fixed *and* documented. Over and out.

--
Viktor.

[Kurt Roeckx]

On Fri, Feb 26, 2016 at 05:34:14PM +0000, Viktor Dukhovni wrote:

> On Fri, Feb 26, 2016 at 05:29:26PM +0000, Salz, Rich wrote:
>
> > As just about the only team member who trolls through RT and closes things
> > with any quantity, I am not sure that I agree that fixing a bug requires
> > documentation if the API isn't already documented.
>

> Focus on fixing bugs in documented interfaces first, and even then review
> the documentation, to make sure the fix comports with the documentation and
> that the latter is not stale.
>
> Once all the bugs in documented interfaces are fixed, I'm afraid
> we really must start documenting the code we're updating. Otherwise,
> there's no way to know we're doing it right, and we're digging the
> hole deeper. New code needs to be documented as it is written,
> old code also, but at a slower pace.

I have to agree with Viktor. I've also been requesting minimal
documentation for the new non-public functions. If I'm changing
something in any non-documented function I try documenting it,
but sometimes I might forget that, and I hope someone would remind
me in that case.


Kurt

[J Mohan Rao Arisankala]

On 2016-02-26 16:50:27 +0000, Stephen Henson via RT said:

> On Wed Feb 24 12:07:05 2016, mo...@computer.org wrote:
>> Hi,
>>
>> I have PR https://github.com/openssl/openssl/pull/739 with the below
>> changes, please have a look.
>>
>> - In EC_KEY_priv2buf(), check for pbuf sanity.
>> - If invoked with NULL, gracefully returns the key length.
>>
>
> If you're doing this you're probably using the wrong API. EC_KEY_priv2buf()
> allocates a buffer and returns its length. If you just want the length and/or
> want to allocate a buffer yourself you should be using EV_KEY_priv2oct()
> instead.

I see your point. So, application deserves a crash in this case for not
using the API as expected.

Thanks,
Mohan

[Jeffrey Walton]

>> Correct me if I am wrong... API's that start with capitol letters are
>> public. Private interfaces use lowercase letters.
>> Documented/undocumented does not really factor things.
>
> You're wrong. Once OpenSSL's past sins are remediated, public
> interfaces are precisely those that are documented. For now, public
> interfaces are either macros or functions and global variables
> those whose symbols are exported by the libssl and libcrypto shared
> libraries. This is not a good place to be.

Please ensure this is documented somewhere. I'm having trouble finding
information on the new rules.

There's 15 or 20 years of using capitol and lower case identifiers to
denote public and private APIs with OpenSSL. For example, see
https://marc.info/?l=openssl-users&m=102683340223621 .

I'm happy to tell folks that no longer applies, but we need to know
what the new rules are is and when the cut-over occured.

Jeff

[Viktor Dukhovni]

> On Feb 27, 2016, at 7:42 PM, Jeffrey Walton <nolo...@gmail.com> wrote:
>
> Please ensure this is documented somewhere. I'm having trouble finding
> information on the new rules.
>
> There's 15 or 20 years of using capitol and lower case identifiers to
> denote public and private APIs with OpenSSL. For example, see
> https://marc.info/?l=openssl-users&m=102683340223621 .
>
> I'm happy to tell folks that no longer applies, but we need to know
> what the new rules are is and when the cut-over occured.

There are no new *rules*, just new goals I'm promoting in this thread.
If it is not documented it is not a fully implemented public interface.
The documentation is part of what it takes to be a first-class
public interface. New code must be documented, and patches to
existing code should bring the documentation up to date. In good
part because it is hard to know whether a patch is correct when
updating undocumented code, and the exercise of documenting it makes
one think harder about the requisite semantics.

The upper-case lower-case thing is a legacy crutch, that is likely
to continue to be adhered to for some time, but increasingly it is
the documentation that must delineate between public and internal
interfaces.

--
Viktor.

[Daniel Kahn Gillmor]

On Fri 2016-02-26 18:04:43 +0100, Viktor Dukhovni <openss...@dukhovni.org> wrote:
> I'd like to propose a policy of no bug fixes to undocumented public

> interfaces. If the interface is useful enough to fix, it has to be
> documented.

fwiw, i agree with Viktor on this proposal. Clear, sane APIs require
documentation, and just writing up the documentation can sometimes spur
people to evaluate the functionality differently and look for bugs in
new ways.

If anyone understands a function well enough to fix bugs in it, you should
understand it well enough to write minimal documentation.

--dkg

[Jeffrey Walton]

Thanks Viktor.

Here's the practical problem I am trying to solve. Its a policy and
procedure problem.

Suppose an organization has a rule that says, "no private APIs shall
be used". How do I tell an organization to differentiate between
public and private APIs to ensure compliance with the policy? What do
I tell QA to verify compliance with the policy?

In the past, we knew from the upper-case lower-case thing. I'm
guessing that held until OpenSSL 1.0.2. I'm also guessing that's is
going to change at 1.1.x.

What do we use now? What are the actionable items or prescriptive
items we can pivot on?

For what its worth, this is your sandbox and I'm not trying to take
any of your toys away. You're free to do what you want. The point here
is it would be very helpful (necessary?) to know what's going on for
organizational policies and procedures.

Jeff

[Viktor Dukhovni]

> On Feb 28, 2016, at 12:17 PM, Jeffrey Walton <nolo...@gmail.com> wrote:
>
> Thanks Viktor.
>
> Here's the practical problem I am trying to solve. Its a policy and
> procedure problem.
>
> Suppose an organization has a rule that says, "no private APIs shall
> be used". How do I tell an organization to differentiate between
> public and private APIs to ensure compliance with the policy? What do
> I tell QA to verify compliance with the policy?

With respect to OpenSSL, such a policy is untenable for applications that
use advanced under-documented portions of the API. OpenSSL does not
currently provide a comprehensive definition of the public API.

For applications that use only the most commonly used features, the best
bet is to only use documented interfaces. If you're using undocumented
features, you're going out on a limb. If you're using something undocumented,
but clearly important and broadly useful, contribute documentation!

--
Viktor.

[Salz, Rich]

FWIW, I agree with Viktor.

[Kurt Roeckx]

On Sun, Feb 28, 2016 at 12:17:41PM -0500, Jeffrey Walton wrote:
> On Sun, Feb 28, 2016 at 12:18 AM, Viktor Dukhovni
> <openss...@dukhovni.org> wrote:
> >
> >> On Feb 27, 2016, at 7:42 PM, Jeffrey Walton <nolo...@gmail.com> wrote:
> >>
> >> Please ensure this is documented somewhere. I'm having trouble finding
> >> information on the new rules.
> >>
> >> There's 15 or 20 years of using capitol and lower case identifiers to
> >> denote public and private APIs with OpenSSL. For example, see
> >> https://marc.info/?l=openssl-users&m=102683340223621 .
> >>
> >> I'm happy to tell folks that no longer applies, but we need to know
> >> what the new rules are is and when the cut-over occured.
> >
> > There are no new *rules*, just new goals I'm promoting in this thread.
> > If it is not documented it is not a fully implemented public interface.
> > The documentation is part of what it takes to be a first-class
> > public interface. New code must be documented, and patches to
> > existing code should bring the documentation up to date. In good
> > part because it is hard to know whether a patch is correct when
> > updating undocumented code, and the exercise of documenting it makes
> > one think harder about the requisite semantics.
> >
> > The upper-case lower-case thing is a legacy crutch, that is likely
> > to continue to be adhered to for some time, but increasingly it is
> > the documentation that must delineate between public and internal
> > interfaces.
>

> Thanks Viktor.
>
> Here's the practical problem I am trying to solve. Its a policy and
> procedure problem.
>
> Suppose an organization has a rule that says, "no private APIs shall
> be used". How do I tell an organization to differentiate between
> public and private APIs to ensure compliance with the policy? What do
> I tell QA to verify compliance with the policy?
>

> In the past, we knew from the upper-case lower-case thing. I'm
> guessing that held until OpenSSL 1.0.2. I'm also guessing that's is
> going to change at 1.1.x.
>
> What do we use now? What are the actionable items or prescriptive
> items we can pivot on?

Those that are in the public headers files are the ones that are
(probably) meant to be public. They also end up in the .num files
and are now the only exported functions on platforms that support
that.

But before you use those functions that are meant to be public
check that they are actually documented. If they're not
documented, try to get them documented before you use them. If
you already use them but they're not documented, try to get them
documented or avoid using them.


Kurt