In particular, verify that the generated key's public point is never the point at infinity.

WE should juse use memset_s. If necessary, we can supply a default implementation of memset_s for platforms that don't provide it, if any. Alternatively, we can drop support for older versions of compilers that don't supply it.

void OPENSSL_cleanse(void *ptr, size_t len) {
#if defined(OPENSSL_WINDOWS)
    SecureZeroMemory(ptr, len);
#else
    memset(ptr, 0, len);

#if !defined(OPENSSL_NO_ASM)
  /* As best as we can tell, this is sufficient to break any optimisations that
     might try to eliminate "superfluous" memsets. If there's an easy way to
     detect memset_s, it would be better to use that. */
  __asm__ __volatile__("" : : "r"(ptr) : "memory");
#endif
#endif  /* !OPENSSL_NO_ASM */
}

See https://boringssl.googlesource.com/boringssl/+/f8e9dcaeea342292bb5ffb27554187d66318ed0f, especially the comment about fat binaries.

Rust doesn't have implicit widening, so casts that are lossy (not necessarily safe) look just like casts that are 100% safe. Instead of using the as operator for widening casts, we should use (and create, if necessary) functions that safely do widening conversions, and use those. Then the use of as would be limited to only those functions.

This depends on #4.

Most ring features' documentations has a "C analogs" section that documents what C API feature is analogous to the Rust API feature. Soon we'll have similar sections mapping the golang and JavaScript WebCrypto APIs to the ring API.

The rustdoc search feature doesn't seem to index these cross-references. We should make some automated tool to make that indexing happen.

Currently, the build exits with errors on OS X.
In trying to get this to build, so far I have changes in the mk\ring.mk:
...
-$(RING_LIB): ARFLAGS = cDrs
+$(RING_LIB): ARFLAGS = crs
...
-PERLASM_x86_ARGS = elf -fPIC -DOPENSSL_IA32_SSE2
-PERLASM_x86_64_ARGS = elf
+PERLASM_x86_ARGS = macosx -fPIC -DOPENSSL_IA32_SSE2
+PERLASM_x86_64_ARGS = macosx
...

The first is because there is not a deterministic flag for ar on OSX, the second to build to the Mac platform.

Also, apparently --gc-sections is deprecated now on OSX, I saw it mentioned[1] that -dead_strip should replace it.
mk/top_of_makefile.mk
-LDFLAGS += -Wl,--gc-sections
+LDFLAGS += -Wl,-dead_strip

[1] http://stackoverflow.com/questions/24734409/make-error-in-mac-clang-ld-unknown-option-gc-sections

After making those changes, I was able to successfully build (x86 and x86_64) on OSX and get a 'PASS' on the various tests I tried in the bin/crypto folder.

-cem

Looking at badboy/nobsign#1, it is obvious that the HMAC API is a little off, in the case where HMAC is being used to sign something we created, pass it around, get it back, and then verify that the thing was something created by us. In particular, ring's current HMAC API is optimized for protocols like SSH and TLS where you we verify things signed by another party, and where we want to make sure we don't use use the same key for signing stuff we send that we use for verifying stuff that the other party sends.

I think we can improve the API here by simply adding:

pub fn verify_self_signature(key: &SigningKey, data: &[u8], sig: &[u8])

It would be just like ring::hmac::verify, except taking a SigningKey instead of a VerificationKey. The name would make it clear that this is to only be used for self-signatures.

I wonder if the AEAD interface needs something similar.

We should be doing runs with the tests running under ASAN.

It may be easier to just do the Android tests in the emulator. There are also some CI services for Android that can test on real devices.

It may be the case that only one of the two implementations is necessary. In theory the binary inverse algorithm should be faster for smallish N, but it is probably worth verifying at some point that this implementation of it actually is faster in some important situation (platform * algorithm). And/or it is worth considering whether the code needs to support even N, or whether it can restrict itself to odd N only so that the binary inversion can always be used. I have a faster implementation of binary inversion sitting around somewhere, so the latter may be a preferable solution. Either way, it would be good for code size to remove one of the implementations.

The appveyor build logs report this. This doesn't seem to have any adverse effects, but we should fix it.

LINK : warning LNK4098: defaultlib 'MSVCRTD' conflicts with use of other libs; use /NODEFAULTLIB:library

Probably the best route is to make a configuration parameter that mk/Windows.props observes for overriding the default runtime library selection.

Note that Rust always links to MSVCRTD.

The BoringSSL EVP signature verification tests were removed because they depended on crypto/pem, which was removed, and because we didn't have anything like crypto/test/file_tests.cc. However, now we have file_tests.rs, and libwebpki's tests in signed_data.rs have a PEM parser in Rus tthat we can use for this, so it should be easy to resurrect the BoringSSL test cases.

As the BoringSSL tests only cover a few scenerios, we should probably add additional cases. It may be a good idea to copy/move some of the non-x.509-specific test cases from libwebpki's signed_data.rs to ring.

After the removal of ecdsa_test.cc, we won't have any test vectors testing, e.g. verification using the wrong key. We should add such test vectors back in, using the Rust testing framework.

From the Rust coding guidleines:

// Extract the contents of Ok variant; fail if Err
fn assert(self) -> V

// Extract the contents of Err variant; fail if Ok
fn assert_err(self) -> E

The code has been using assert(x.is_ok()) and assert(x.is_err()) instead.

I asked Adam Langley why BoringSSL still has the RSA blinding code even though the BoringSSL RSA code is supposed to be constant-time. The response was "I don't think that I'm confident enough in the constant-timeness of the rest of the RSA code to remove blinding." Especially given experience with the ECC code, I agree with him for the time being. But, we should find a way to become confident (fixing the code if necessary) that the constant-time RSA implementation is constant time. Then we should remove the blinding code.

e.g. Control Flow Integrity, stack protection, etc.

It would be worth having the build system run one of the scanning tools to check that these flags are all set correctly.

Currently rustdoc doesn't linkify cross-references that are done by mentioning an item with backtick quotes. Until rustdoc supports that feature, we should just do the linkification ourselves.

In particular, stop using it as the iteration count for PBKDF2. Otherwise, it won't work correctly on 16-bit-pointer platforms like AVR.

...if/when test build scripts are added. See rust-lang/cargo#1581.

This will reduce building time for the common case where ring is used as a library in another project and the ring tests are never run.

https://www.tarsnap.com/scrypt.html.

In particular, we should provide an scrypt implementation that can implement the Android FDE cryptosystem. See https://source.android.com/devices/tech/security/encryption/#storing_the_encrypted_key, which describes the updates from http://nelenkov.blogspot.com/2014/10/revisiting-android-disk-encryption.html.

Note that Android's FDE seems to require outputting TWO blocks, but currently we only support outputting ONE block. So, we'll probably have to extend the PBKDF2 implementation to support larger outputs.

Until this is fixed in rustdoc, we should fix this ourselves with some post-processing tool.

Looking at the proposed new libc and considering our current usage, it becomes pretty obvious we don't need to depend on the libc crate and we're better off not doing so. Our needs are so simple we can get away with just defining a couple of types in ring::ffi. If we need a more advanced thing, then we can look at rust-c99.

Note that we have to define "side-channel-free," and we might have different definitions of "side-channel-free" for different configurations.

Currently there are some test cases in pbkdf2_tests.txt that are commented out because they are invalid inputs for ring's PBKDF2 module. In particular, they request an output longer than the output length of the the digest function being used. We should turn those test cases into the equivalent of [should_panic]. AFAICT, this means we need to:

1. Uncomment those test cases.
2. Annotate those test cases with "FAILS: OUTPUT_LARGER_THAN_DIGEST_OUTPUT".
3. Change the testing code so that for the cases marked "FAILS: OUTPUT_LARGER_THAN_DIGEST_OUTPUT", a task gets spawned that does the test case. We have to wait for the task to end and then verify that it was ended due to a panic!. In other words, we need to create the equivalent of a [should_panic] test.

Additionally, we should add more test cases, particularly a test case for iterations == 0, as BoringSSL has (had) a bug when interations == 0.

AFAICT, this is complicated by rust-lang/rust#25869, as I believe we cannot do [should_panic] tests or create a data-driven alternative to [should_panic] on Win32 since Rust on Win32 doesn't implement panic! correctly; `panic!`` just kills the entire process. We should probably just work around that by not running these tests on Win32.

libwebpki has the Input/Reader framework in webpki::input that is used to safely parse input. Factor that framework out of libwebpki so that ring can use it. Then, all inputs that need to be parsed should bed passed as instances of Input or Reader, instead of &[u8] as they are now.

See openssl/openssl@5e5ece5 and openssl/openssl@a93d3e0

Cortex-M4, Cortex-M3, Cortex-M0, Cortex-M0+, Cortex-M1.

See https://llvm.org/bugs/show_bug.cgi?id=15393. Building with debug info breaks the Windows build, so we can't build with debug = true yet.

For example, why doesn't ring have a DigestAlgorithm trait that is implemented by types named SHA256, SHA384, etc.? Why is everything dynamically dispatched instead of parameterized on types and statically dispatched? How does the current design avoids unnecessary heap usage and minimizes code size. That means we have to verify that there is actually a code size savings relative to the type parameterization and static dispatching approach that people seem to expect in Rust.

BoringSSL and OpenSSL attempt to do zeroization of key material. Should ring? In particular, should we zeroize HMAC key material?

crypto/cmac is being removed because it depends on the non-AEAD cipher.h encryption interface, which is being removed. In order to implement the CCM AEAD, we need a new CTR + CBC-MAC (CCM) implementation. The new CCM implementation can assume that AES is the only cipher that it will be used with, and that it will only be exposed by the ring::aead interface, which should simplify the new implementation. [Edit: Say "CTR + CBC-MAC" instead of "CMAC".]

See http://www.methics.fi/technology/signature-security/ and the stuff it cites.

At the very least, we should require callers to pass in the minimum key size they support.

In the upcoming generic signature and key exchange interfaces, that might mean that we need to define separate instances of Algorithm for various minimums. e.g. RSA_PKCS1_AT_LEAST_2048_BITS, RSA_PKCS1_AT_LEAST_1024_BITS, etc.

It takes a fair amount of code to support these non-optimal nonce lengths. Very few applications will need to support these non-standard nonce lengths and it isn't worth the effort to test the code to verify that it works correctly for them. The ring::aead interface is much simplified if the nonce length is fixed per ring::aead::Algorithm instance.

It looks like it will be easier to do this if the non-AEAD AES-GCM interface in crypto/cipher/e_aes.c is removed first.

See Chromium issue https://code.google.com/p/chromium/issues/detail?id=535758.

64-bit at least, probably 32-bit too.

Is there any reason this is not yet published on crates.io?

C:\msys64\home\Peter\ring> cargo build
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling ring v0.1.0 (file:///C:/msys64/home/Peter/ring)
   Compiling rustc-serialize v0.3.16
   Compiling libc v0.1.10
Build failed, waiting for other jobs to finish...
failed to run custom build command for `ring v0.1.0 (file:///C:/msys64/home/Peter/ring)`
Process didn't exit successfully: `C:\msys64\home\Peter\ring\target\debug\build\ring-761370bdb05fc17f\build-script-build` (exit code: 101)
--- stdout
APPDATA: C:\Users\Peter\AppData\Roaming
PROMPT: $P$G
HOMEPATH: \Users\Peter
OPT_LEVEL: 0
COMMONPROGRAMW6432: C:\Program Files\Common Files
VS120COMNTOOLS: C:\Program Files (x86)\Microsoft Visual Studio 12.0\Common7\Tools\
COMPUTERNAME: WIN81-SSD
PROCESSOR_IDENTIFIER: AMD64 Family 21 Model 19 Stepping 1, AuthenticAMD
PROGRAMW6432: C:\Program Files
: C:=C:\msys64\home\Peter
OUT_DIR: C:\msys64\home\Peter\ring\target\debug\build\ring-761370bdb05fc17f\out
PROCESSOR_LEVEL: 21
LOCALAPPDATA: C:\Users\Peter\AppData\Local
TARGET: x86_64-pc-windows-msvc
PROCESSOR_REVISION: 1301
USERDOMAIN_ROAMINGPROFILE: WIN81-SSD
CARGO_PKG_VERSION_MINOR: 1
COMMONPROGRAMFILES(X86): C:\Program Files (x86)\Common Files
HOMEDRIVE: C:
PSMODULEPATH: C:\Users\Peter\Documents\WindowsPowerShell\Modules;C:\Program Files\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules\
PATHEXT: .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.CPL
PROGRAMDATA: C:\ProgramData
VS110COMNTOOLS: C:\Program Files (x86)\Microsoft Visual Studio 11.0\Common7\Tools\
CARGO_PKG_VERSION_PATCH: 0
WINDIR: C:\WINDOWS
FPS_BROWSER_USER_PROFILE_STRING: Default
PROGRAMFILES(X86): C:\Program Files (x86)
NUM_JOBS: 4
SESSIONNAME: Console
VBOX_MSI_INSTALL_PATH: C:\Program Files\Oracle\Virtualbox\
HOST: x86_64-pc-windows-msvc
PUBLIC: C:\Users\Public
PATH: C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem;C:\WINDOWS\System32\WindowsPowerShell\v1.0\;C:\Program Files\Microsoft SQL Server\110\Tools\Binn\;C:\Program Files (x86)\Microsoft SDKs\TypeScript\1.0\;C:\Program Files (x86)\Windows Kits\8.1\Windows Performance Toolkit\;C:\Program Files (x86)\ATI Technologies\ATI.ACE\Core-Static;C:\Program Files (x86)\AMD\ATI.ACE\Core-Static;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\;C:\Program Files (x86)\nodejs\;C:\WINDOWS\system32\config\systemprofile\.dnx\bin;C:\Program Files\Microsoft DNX\Dnvm\;C:\Program Files (x86)\Skype\Phone\;;C:\Users\Peter\AppData\Local\Code\bin;C:\Users\Peter\AppData\Roaming\npm;C:\rust64-msvc\bin;C:\Program Files (x86)\CMake\bin;C:\Program Files (x86)\Git\bin;C:\msys64\home\Peter\ring\target\debug\deps
SYSTEMDRIVE: C:
USERPROFILE: C:\Users\Peter
CARGO_PKG_VERSION_PRE:
VS140COMNTOOLS: C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\
FPS_BROWSER_APP_PROFILE_STRING: Internet Explorer
NUMBER_OF_PROCESSORS: 4
TEMP: C:\Users\Peter\AppData\Local\Temp
SSH_AGENT_PID: 748
HOME: C:\Users\Peter
CARGO_PKG_VERSION_MAJOR: 0
DXSDK_DIR: C:\Program Files (x86)\Microsoft DirectX SDK (June 2010)\
VSSDK140INSTALL: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VSSDK\
USERNAME: Peter
SSH_AUTH_SOCK: /tmp/ssh-l2ycI5SLhjmF/agent.3796
CARGO_PKG_VERSION: 0.1.0
COMMONPROGRAMFILES: C:\Program Files\Common Files
OS: Windows_NT
PROCESSOR_ARCHITECTURE: AMD64
DEBUG: true
USERDOMAIN: WIN81-SSD
PROFILE: debug
CARGO_MANIFEST_DIR: C:\msys64\home\Peter\ring
SYSTEMROOT: C:\WINDOWS
COMSPEC: C:\WINDOWS\system32\cmd.exe
LOGONSERVER: \\MicrosoftAccount
ALLUSERSPROFILE: C:\ProgramData
FP_NO_HOST_CHECK: NO
PROGRAMFILES: C:\Program Files
TMP: C:\Users\Peter\AppData\Local\Temp

--- stderr
thread '<main>' panicked at 'failed to execute msbuild: The system cannot find the file specified. (os error 2)', build.rs:87

I have VS 2015 installed, so it would be nice if ring could find where msbuild is.

When using your proposed code directly (badboy/nobsign/issues/2) I hit a bug crashing (and coredumping) the compiler.

This little piece of Rust reproduces it:

extern crate ring;

static _ALGORITHM: &'static ring::digest::Algorithm = &ring::digest::SHA1;

pub fn bug() {
    vec![0u8; ring::digest::SHA1.digest_len];
}

Compiling that (add this repo as a dependency to a crate, then cargo build) leads to:

rustc: /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/lib/IR/Constants.cpp:2044: static llvm::Constant* llvm::ConstantExpr::getGetElementPtr(llvm::Type*, llvm::Constant*, llvm::ArrayRef<llvm::Value*>, bool, llvm::Type*): Assertion `DestTy && "GEP indices invalid!"' failed.

and a coredump.

@Kimundi was so nice to explain a bit:

library does wrong unsafe things => invariants are violated => rust compiler emits IR with UB to llvm => llvm in debug mode checks somke of the invariants and triggers an assertion error => assertion do abort the process => abort is implemented as a invalid instruction => core dump

It seems that this usage of references to digest::SHA1 (which itself is a static) leads to this undefined behavior.

Of course it might be nice if rustc can prevent this.
But nevertheless ring should try to provide safe abstractions.

This means using std::ptr::Unique for things like
ECDHEphemeralKeyPair. We should probably have some helper in ring::ffi that wraps a pointer with std::ptr::Unque, implements Send, and also implements Drop by calling a one-argument C function to delete the object. That's particularly true because std::ptr::Unique is unstable, so we can't actually use it.

See also https://internals.rust-lang.org/t/pre-rfc-raw-pointer-cleanup/2544.

See rust-lang/rust/#28096.

There are some casts like digest.len() as libc::size_t that are potentially truncating and thus unsafe. At the same time, the comments in the code questioning the safety of such casts need to be removed.

This will have to wait for a higher-versioned released of the libc crate with rust-lang/rust#28096 merged into it.

Right now, everything is being exported, which is bad.

See the checked_struct branch, particularly, cd53d14.

That is incomplete in that it doesn't address alignment from the Rust side. In particular, we assume that the alignment of u64 is 8 bytes, but we don't assert that.

It is also incomplete because it doesn't do the FFI checking for ring::digest.

1. The "init" functions just initialize the context with constant per-algorithm values. That can be done with a per-algorithm (static) initial context value array and a single init function that uses memcpy to copy that initial value into the context, avoiding the need for per-algorithm init functions. Then we can delete the C code for the init functions.
2. The streaming interface requires the context to buffer data until an entire block of data is available. Right now, every digest implementation implements its own copy of this logic in C. Instead, w should implement one copy of tthe buffering logic in Rust and remove the C _Update and _Finalfunctions, leaving only the C _Transform functions for Rust to call into.