One week of OpenSSL cleanup | Hacker News Reader

One week of OpenSSL cleanup

undeadly.org

47 pointsch0wn47 comments

They still haven't done the most critical thing: add tests. This is for two reasons: the code has no tests now, and when you refactor, tests are important to make sure you're not subtly introducing errors.

If openssl has no tests, what is it doing when you run "make test"?

I'd love to see information about the actual security value of these changes. A ton are just adjusting to KNF.

http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/log/lib/li...

Speaking as someone who spends a big chunk of his working life auditing others code, having a consistent style makes it a lot easier to spot problems.

But isn't what's happening here a change to the coding style to match the style used by BSD? That might be fun for BSD coders, but is that actually helping improve the OpenSSL code?

It might not help upstream OpenSSL, unless they keep an eye out for the bug fixes. Consider OpenBSD the upstream for this new fork.

I don't think they intend to contribute back to the original OpenSSL code base, but see this as a OpenBSD only fork.

man style

http://www.openbsd.org/cgi-bin/man.cgi?query=style&apropos=0...

Well, that'd be useless to move from tabs to spaces or from n spaces to m spaces in this context, but as far as I've observed, OpenSSL codebase uses the unconventional Whitesmiths style [1][2][3], which might be a burden on the developers to navigate.

[1] http://en.wikipedia.org/wiki/Indent_style#Whitesmiths_style

[2] http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/src/cry...

[3] http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/src/cry...

edit: formatting

That's pretty disappointing; a big formatting change like that means that it'll increase friction of pulling fixes and changes between the two versions.

But it makes it readable, which makes it easier to find bugs. This is vastly more important.

This, very much.

Normally I try to avoid getting into arguments about style and such, it's just bikeshedding. But if the OpenBSD people are going to clean up, review, audit, fix, improve and maintain this code, it makes all the sense to switch to a style they are used to and comfortable with. It also makes sense to nuke all the junk that gets in the way and which they will not and cannot test because it is not at all relevant to the platform.

Besides, the OpenSSL code is a fucking mess.

At some point you need to say, "This code is a mess, and pretty much anything we do to it will be an improvement."

I spent a couple hours with the OpenSSL source last week, and agree that a major cleanup is necessary. This means

- Rip out unused stuff - Rip out stuff that obviously doesn't work - Make big cleanups

I've done this in the past with token-level tools to ensure that I wasn't breaking things.

OpenSSL desperately needed to have a bunch of garbage removed. It needs good tests, it needs to be slimmed down and made clean so that the code is readable and maintainable.

Frankly I wouldn't trust fixes to the old version anyway, given its state.

> At some point you need to say, "This code is a mess, and pretty much anything we do to it will be an improvement."

In software development it's often the case that the first claim is true, but the second isn't.

> Frankly I wouldn't trust fixes to the old version anyway, given its state.

It will be interesting to see if this get organized and properly funded, given this library's importance to international commerce.

> Frankly I wouldn't trust fixes to the old version anyway, given its state.

Clean slate! An admirable goal, and clearly the best solution. Let's see if this actually happens.

There have been a few bugfixes. Some insecure idioms have been corrected, a broken implementation of calloc (seriously, why?) has been replaced with calls to the library function. Some deceiving (and/or just useless) wrappers have been eradicated. More significant is the removal of the built-in RNG and a few hilarious (or sad) questionable "features" relating to its use.

I don't think the changes they are doing at the moment have much security value. They aren't working on fixing OpenSSL security flaws. They are obliterating all the layers of cross-platform compatibility that make the code more complex, and removing everything OpenBSD isn't likely to use.

They are transforming a local copy of openssl that tracks upstream changes to their own fork, that probably won't reunite with the upstream version anytime soon.

In the process, they are obviously likely to find some bugs (and they already have fixed one or two harmless ones), but I think they are more interested in building a clean fork for themselves than in fixing the upstream openssl.

But they are doing stuff like this:

1. Let's remove OPENSSL_cleanse: http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/commit/lib...

2. Crap. Let's put it back again: http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/commit/lib...

That plus the 'tone' of the commits makes me wary of these changes.

Did you actually read the second diff you linked? They did not put anything back again. They removed even more junk. It is all deletions.

I'm getting tired of all the people who complain about the humour, tone or style in the commit messages. If you have serious concerns, why not read the diffs and comment on the actual changes? This is how real problems get noticed and corrected.

But some things were in fact removed and put back again, because it turns out some ports depended on it. Sometimes you just don't notice what a few thousand other applications depend on before you remove some code.

You're right. I'm wrong about number 2. Mea culpa.

Also look at the manpage for explicit_bzero: http://www.openbsd.org/cgi-bin/man.cgi?query=bzero&sektion=3

It already does what OpenSSL_cleanse is meant to do, so using it makes sense.

You are seeing this because they are working in public. Any of us would look like fools if you saw our intermediate work product.

OpenSMTPD, OpenNTPD and OpenSSH are avail for other platforms. They will strip OpenSSL down to essentials, spend months auditing what's left then when satisfied it is reliable software porting to other platforms can begin. It's only been a week

Looks like significant amount of non-stylistic changes too:

http://opensslrampage.org/

The OpenBSD team just want to do it their own way, regardless of the practical value of creating single solutions that work everywhere. OpenSSL was a great idea back when there was no other open crypto library. But now amateur cryptographers are a dime a dozen and anyone can slap together something that just works for them and disregards others. Hence stripping out everything, changing APIs, and enforcing KNF. There's probably no security value to 90% of the changes; they just want conformity to their personal style.

As feld, clarry, danrik, etc. have pointed out, having a consistent coding style makes identifying bugs much easier.

"regardless of the practical value of creating single solutions that work everywhere"

Since these are the same folks who do OpenSSH, I would imagine after the initial work is done, it will probably evolve to be as portable as OpenSSH and the other OpenBSD projects.

Also, they are doing many bug fixes as shown on http://opensslrampage.org

The API's aren't changing... they are keeping API compat. Sure you won't be able to change the PRNG anymore ... but that isn't that big of a deal.

This isn't about confirming to their personal style, this is about cleaning the code up so that auditing it is possible.

I would have loved knowing their reasoning behind this decision, which I'm sure it's very calculated, because the implications are huge. They're choosing to switch from a simple packager role to a maintainer role, they will have to always keep their fork in sync with upstream which if they try to apply every changes will require a lot of work, or maybe they have decided on another strategy e.g. freezing functionalities and applying only security patches, I don't know I didn't look at the current types of clean-ups maybe that could hint what their intentions are.

Regardless, I have mixed feeling about this, one month ago a lot of people lauded openssl for not being vulnerable to the goto fail saga while criticizing Apple for not using openssl and now everyone is ready to ditch openssl in a heartbeat for a flaw not related to crypto, not committed by core developers. I'm not defending openssl, I would love a concerted effort by big industry players for implementing a new SSL/TLS stack from scratch, maybe in a safer language, but I also don't think it is a reason for taking quick, disruptive decisions like this. It is also likely they had this move in mind before this episode and this flaw was the last straw but they should have exposed their plan, it would have been a bit better for us to understand their reasoning.

I cannot speak for the people who made this decision.

But if you look at the code, you might come to the conclusion that the upstream is not so focused code quality (security!) as one might hope. There is such a jungle of code that can only be justified by the desire for more features, more portability (to crazy ancient platforms too), more speed, more flexibility... it's all more knobs and code where real security bugs can lurk. And code paths that do not get tested properly. You can also find many outright dangerous idioms used throughout. Does that mean the upstream is ignorant, or that they do not care, or that they do not have the man power to fix their aging code? Either way, it's bad.

The OpenBSD developers have different priorities, and they believe they can do better. As such, not being able to blindly pull changes and updates from the upstream developers they do not trust does not look like a significant loss. If new features are to be merged in, it has to be properly reviewed and adapted. Yes, it's more work that way. But the end result is likely better code.

the talk about portability to crazy ancient platforms is weird. Didn't openbsd ask for 20K$ to keep up a build farm of ancient platforms?

http://marc.info/?l=openbsd-tech&m=138973312304511&w=2

They use that old equip to find bugs

I think they asked for $20k for old hardware (like vax), not old platforms (like VMS).

It is one thing to make an OS support the hardware and run on a "weird" piece of hardware. But the removed code in OpenSSL is about making it compile and run on systems on which the standard (defacto or specified) APIs do not exist or do not behave like they should, and which have broken compilers that need workarounds. All of these add conditional code paths that won't get the amount of testing they need, and which will only hinder developers when they try to understand and improve the surrounding code.

Also, the $20k was for the power bill. Yes, there are some older systems among the powered ones. But there's more, there's really a rack full of hardware running the infrastructure that makes OpenBSD available to us. There's networking gear, there's AC...

Dobbsbob makes a good point too. The weird hardware OpenBSD runs on helps find bugs; the untested code paths in OpenSSL add bugs.

Those ancient platforms/architectures run OpenBSD, though, not the original OS. Nobody in their right mind should run those dead OSes on the public internet; why bother supporting them in the latest OpenSSL release?

"...for a flaw not related to crypto..."

The thing about crypto is that almost any bug in a crypto library is related to crypto. It can't be easily isolated, and if you try, you have to actually analyze the isolation to make sure that it's really isolated.

"...disruptive decisions..."

Who is it disrupting?

OpenBSD is trying to provide value to their users. If you want slow, steady, incremental, reactive improvements to OpenSSL, every other OS will give you that. OpenBSD apparently believes its users would benefit from a proactive approach to security.

Maybe they should just fork it completely, keep only the most used stuff, and call it OpenTLS - the "clean" alternative to OpenSSL. Then push for it to get accepted in server software. It would be kind of like the OpenOffice/LibreOffice situation.

Although if they're going to embark on such an endeavour, maybe it would be best to also rewrite it completely in a memory safe language like Rust (but in a year or two, after Rust becomes more mature).

It wouldn't help if they create another project based on OpenSSL code, because that will inherit OpenSSL's license that everybody hates, and wouldn't necessarily help other projects adopt it. It might also discourage other people from looking at / contributing code, when they could just improve another TLS library with a saner license that people actually want to use not just due to legacy.

I would venture to speculate that not many OpenBSD developers are familiar with Rust. They are familiar and very practiced at writing secure C.

They maintained an audited fork of Apache 1.3 for a number of years (I think recently switched or will switch to nginx), so it's not unprecidented.

> They're choosing to switch from a simple packager role to a maintainer role, they will have to always keep their fork in sync with upstream which if they try to apply every changes will require a lot of work, or maybe they have decided on another strategy e.g. freezing functionalities and applying only security patches, I don't know I didn't look at the current types of clean-ups maybe that could hint what their intentions are.

In fairness it's always been my impression that a BSD distribution is intended to be much more than a mere packager of software, but instead an integrator of those libraries into a sane, coherent overall operating system.

This is the reason OpenSSH has their native version for OpenBSD and then a "portable" version for the rest of the POSIX world.

  > They're choosing to switch from a simple packager role to a maintainer role.

This is hardly a leap of faith for the OpenBSD developers. The dev team has always placed an emphasis on security even if the added security meant significant additional work. OpenBSD has a history of creating new software products to replace products that they deemed unacceptable from a security standpoint (including forking netbsd). I imagine that in addition to OpenBSD you are familiar with at least one of these projects:

OpenBGPD http://www.openbgpd.org

OpenNTPD http://www.openntpd.org

OpenSMTPD http://www.opensmtpd.org

OpenSSH http://www.openssh.org

This pays off for them too. NTP DoS reflection attack this year didn't work on OpenNTPD which had removed test commands and extensions that were used by attackers. The BGP hijacking going on last year sending traffic to Belarus didn't work on OpenBGPD http://arstechnica.com/security/2013/11/repeated-attacks-hij...

I think the distros shared a lot of blame for the ntp ddos attacks. The defaults were ridiculous. I want to scream every time I see `restrict default kod blah blah` without `limited`. Somewhere someone threw up that terrible restrict statement and it got copied everywhere. KOD without `limited` is useless and a good indicator the maintainer never read the docs: "If the kod flag is used in a restriction which does not have the limited flag, no KoD responses will result." As far as alternatives to the ntp reference implementation go I like Miroslav's chrony, especially for things like laptops with intermittent connectivity and power saving concerns.

In case your cvs-fu is rusty FreshBSD[1] or OpenSSL Rampage[2] provide an easy way of keeping an eye on the changes. One of my favorites so far is: "Remove hacky workaround for Cray T3E," as in the debuted in 1995, Cray Research T3E.[3]

  -/* Added for T3E, address-of fails on bit field (beckman@acl.lanl.gov) */
  -#ifndef BIT_FIELD_LIMITS
          memcpy(&server.sin_addr.s_addr, ip, 4);
  -#else
  -       memcpy(&server.sin_addr, ip, 4);
  -#endif

[^1]: http://freshbsd.org/search?project=openbsd&q=project%3Aopenb...

[^2]: http://opensslrampage.org/

[^3]: http://freshbsd.org/commit/openbsd/01f41ed5b37037b963c0de2c2...

[^3]: https://en.wikipedia.org/wiki/Cray_T3E

There's also http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/log/

I hate cgit's log view, they should at least be honest and call it shortlog. Most importantly the link you provide displays everything from src, I could just browse the source-changes list if I wanted to see everything. The freshbsd and rampage links allow you to see just the libssl changes.

There are tons of simple cleanups I'd love to do. But with all the activity and people already working on it, I'm afraid I'd just be stepping on people's toes if I started sending diffs now.

Stepping on each other's toes is just what happens with large projects. There are tools to help deal with merge conflicts so I don't think you should hold yourself back. OpenSSL needs all the eyes it can get right now.

Reply on news.ycombinator.com