search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Remove `Crystal::Digest` implementations #10346

Open straight-shoota opened 3 years ago

straight-shoota commented 3 years ago

Crystal::Digest::MD5 is an internal implementation used by the compiler, in only one location:

https://github.com/crystal-lang/crystal/blob/33c97d419c88b4c6595786597f0ed04c7973b283/src/compiler/crystal/compiler.cr#L644

But this use case doesn't need MD5 at all. The purpose is to restrict long names to 50 characters while still having a unique identifier. So there's really no need for a cryptographic hash function.

The same can be achieved much simpler. For example String#hash should do as a hash function. Or it could use a counter and names get assigned sequential numeric identifiers.

If we change that, we can remove Crystal::Digest::MD5 from the code base.

There's also Crystal::Digest::SHA1 which has a similar purpose only for the compiler's playground when compiled without openssl.

https://github.com/crystal-lang/crystal/blob/33c97d419c88b4c6595786597f0ed04c7973b283/src/http/web_socket/protocol.cr#L330-L332

Techincally it's also used for any program that uses the websocket implementation is compiled without openssl. But that's not really an intended use case.

Since the playground is only a development tool security concerns really don't matter much. But it seems impossible to avoid because the Sec-WebSocket-Accept header with a SHA1 hashed value is mandatory for establishing a websocket connection.

j8r commented 3 years ago

Ideally any program wanting to use WebSocket should compile with openssl, for security reasons and also performance.

The playground should not be needed to bootstrap the compiler. An option is to not have the Playground at all when openssl is not available. Then, no Digest will be needed anymore.

straight-shoota commented 3 years ago

Compiler builds are usually not linked against openssl (AFAIK because static linking is difficult). It would mean the playground would generally not be available. That's a major change and basically drop the playground from the compiler entirely.

asterite commented 3 years ago

There's simply no rush at all to change this.

j8r commented 3 years ago

You're both right. Then, for the moment, just changing the MD5 use in crystal/src/compiler/crystal/compiler.cr will be good.

I don't know how the playground could possibly use the local host's openssl library after being compiled.

As a side note, if digests are dropped from this repository, could they be moved in a repository?

straight-shoota commented 3 years ago

@asterite No rush, yes. But there can still be an issue to track this.

As a side note, if digests are dropped from this repository, could they be moved in a repository?

This is only about internal implementations. They're not intended for public use. So there's no reason to make it available in separate repo. At least not in crystal-lang, if somebody wants to publish a shard there's noone stopping you. I wouldn't recommend it though.

j8r commented 3 years ago

It would be good for educational purposes, and some non security related cases. Their implementations could even be improved and audited, who knows.

SamantazFox commented 3 years ago

Why would you remove those digests? It's kinda useful to be able to compute a MD5/SHA* sum in order to veryfy a download, in a os-agnostic way and without having to write to file.

Or am I confugisng this with some other implementation?

straight-shoota commented 3 years ago

These are pure Crystal implementations and neither optimized for performance nor security (not that the latter matters too much anymore).

Both digest methods are available in stdlib via bindings to ˋlibcryptoˋ in the ˋOpenSSLˋ namespace. So as long as you can link ˋlibcryptoˋ, you already have a much better implementation available and there's no need for primitive implementation in pure Crystal.