Replace rust-crypto crate with RustCrypto crates #239

newpavlov · 2018-07-23T13:46:01Z

As was discussed in the #128. Currently this PR lacks only AES-CTR in audio/src/decrypt.rs and connect/src/discovery.rs, which will require some work on RustCrypto side.

newpavlov · 2018-07-30T11:27:18Z

Note that aes-ctr requires Rust 1.27 (as it depends on stabilized target features). To have the best performance on modern x86-64 CPUs you'll need to enable aes and ssse3 target features.

ashthespy · 2018-09-23T18:11:53Z

Tested on armv7 and all seems to work well here.
Don't know enough to check performance vs the earlier implementation!

ashthespy · 2018-10-12T22:50:01Z

@newpavlov 1f1cd11 seems to be giving me Bus errors on aarch64 (running armv7hf 32bit binary):

Program received signal SIGBUS, Bus error.
[Switching to Thread 0xf69ff2a0 (LWP 16863)]
0xaab85efc in _$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$::new::h428d9f3e664339b6 ()
(gdb) backtrace
#0  0xaab85efc in _$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$::new::h428d9f3e664339b6 ()
#1  0xaab7f664 in librespot_playback::player::PlayerInternal::run::h11714ef38d488ae5 ()
Cannot access memory at address 0x0
#2  0xf69fcee0 in ?? ()
Cannot access memory at address 0x0
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)

Any ideas how to debug/isoloate this further?

newpavlov · 2018-10-12T23:05:00Z

Hm, I think problem manifests itself in this line, not sure why it happens though. Are you sure that nonce pointer is not null?

UPD: Hm, looking at the code it looks like pointer can not be null...

ashthespy · 2018-10-12T23:15:22Z

Hm, I think problem manifests itself in this line, not sure why it happens though. Are you sure that nonce pointer is not null?

I am a gdb novice - so my knowledge stops at :={
How exactly can I check?

(gdb) frame
#0  0xaab85efc in _$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$::new::h428d9f3e664339b6 ()
(gdb) info f
Stack level 0, frame at 0xf65f7b40:
 pc = 0xaab85efc
    in _$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$::new::h428d9f3e664339b6; saved pc = 0xaab7f664
 called by frame at 0xf65fce70
 Arglist at 0xf65f60b0, args:
 Locals at 0xf65f60b0, Previous frame's sp is 0xf65f7b40
 Saved registers:
  r4 at 0xf65f7b1c, r5 at 0xf65f7b20, r6 at 0xf65f7b24, r7 at 0xf65f7b28, r8 at 0xf65f7b2c,
  r9 at 0xf65f7b30, r10 at 0xf65f7b34, r11 at 0xf65f7b38, lr at 0xf65f7b3c
(gdb)

and

(gdb) x/3ug 0xaab85efc
0xaab85efc <_ZN74_$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$3new17h428d9f3e664339b6E+20>:       16325108307302940720    16540877113332465672
0xaab85f0c <_ZN74_$LT$ctr..Ctr128$LT$C$GT$$u20$as$u20$stream_cipher..NewFixStreamCipher$GT$3new17h428d9f3e664339b6E+36>:       16540877096152596492

ashthespy · 2018-10-12T23:24:37Z

I could not reproduce it with the usage example over at docs. But if you have a better test case to isolate the issue, willing to test!

newpavlov · 2018-10-12T23:26:44Z

Please try to execute aes-ctr tests on your platform. Meanwhile I will write a small replacement for this line, which you will be able to test via [patch].

newpavlov · 2018-10-12T23:41:25Z

Try to do the following:

Clone ctr crate from the repo.
Replace line 105 with this code:

let nonce = [
    ((nonce[0] as u64) << 8*7) + ((nonce[1] as u64) << 8*6) +
    ((nonce[2] as u64) << 8*5) + ((nonce[3] as u64) << 8*4) +
    ((nonce[4] as u64) << 8*3) + ((nonce[5] as u64) << 8*2) +
    ((nonce[6] as u64) << 8*1) + ((nonce[7] as u64) << 8*0),
    ((nonce[8] as u64) << 8*7) + ((nonce[9] as u64) << 8*6) +
    ((nonce[10] as u64) << 8*5) + ((nonce[11] as u64) << 8*4) +
    ((nonce[12] as u64) << 8*3) + ((nonce[13] as u64) << 8*2) +
    ((nonce[14] as u64) << 8*1) + ((nonce[15] as u64) << 8*0),
];

Add [patch] section to librespot's Cargo.toml in which replace ctr crate with your modified copy:

[patch.crates.io]
ctr = { path = 'path/to/modified/ctr' }

Test resulted binary.

ashthespy · 2018-10-12T23:58:13Z

I don't know how to run cargo test when cross compiling. I tried taking the resulting binary from cargo --test --release --target arm-unknown-linux-gnueabihf and it didn't do anything (Ran test 0 of 0)

But this gets interesting - I just added a simple debug to try and recreate a simpler minimal example,

println!("key: {:?}", key.0);
let cipher = Aes128Ctr::new(
            &GenericArray::from_slice(&key.0),
            &GenericArray::from_slice(&AUDIO_AESIV),
        );

and it doesn't have any bus errors now?

Either way - building with the patch now
EDIT: [patch.crates.io] --> [patch.crates-io]

ashthespy · 2018-10-13T00:37:01Z

Your patch works! I couldn't get the [patch] syntax working - I didn't seem to be able to get it to patch dependencies (ctr) of aes-ctr but that is a different matter! :)
Thanks for the quick response!

newpavlov · 2018-10-13T00:41:56Z

Hm, now we need to find if I've maid a mistake and the line introduces UB, or is it Rust miscompialtion. One possible explanation is alignment issues, not sure how to check it though. :/ I will try to ask around.

ashthespy · 2018-10-13T00:45:55Z

Hm, now we need to find if I've maid a mistake and the line introduces UB, or is it Rust miscompialtion. One possible explanation is alignment issues, not sure how to check it though. :/ I will try to ask around.

Naive question - was it just a coincidence that this error didn't manifest when I added the print?
If you have some tests that I can cross compile with my current toolchain, will be happy to test for you!

newpavlov · 2018-10-13T00:49:51Z

Naive question - was it just a coincidence that this error didn't manifest when I added the print?

UB and miscompilation bugs are quite fragile, and can result in big behaviour changes on small code edits.

If you have some tests that I can cross compile with my current toolchain, will be happy to test for you!

I can translate tests to a stand-alone app, but I probably will do it tomorrow.

BTW have you updated Rust to 1.29.2? I doubt that this miscomiplation bug causes this situation, but it's worth to check.

ashthespy · 2018-10-13T00:55:45Z

Fair enough with the undefined behaviour.
I just checked, I was building both with 1.29.1 and 1.29.2 - both of them had the error.

newpavlov · 2018-10-13T14:31:13Z

Try to test the following snippet:

extern crate aes_ctr;

use aes_ctr::Aes128Ctr;
use aes_ctr::stream_cipher::{NewFixStreamCipher, StreamCipherCore};
use aes_ctr::stream_cipher::generic_array::GenericArray;

const AUDIO_AESIV: [u8; 16] = [
    0x72, 0xe0, 0x67, 0xfb, 0xdd, 0xcb, 0xcf, 0x77,
    0xeb, 0xe8, 0xbc, 0x64, 0x3f, 0x63, 0x0d, 0x93,
];

#[inline(never)]
fn new_cipher(key: &[u8]) -> Aes128Ctr {
    Aes128Ctr::new(
        GenericArray::from_slice(key),
        GenericArray::from_slice(&AUDIO_AESIV),
    )
}

#[inline(never)]
fn encrypt(cipher: &mut Aes128Ctr) -> [u8; 10] {
    let mut data = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    cipher.apply_keystream(&mut data);
    data
}

fn main() {
    let mut cipher = new_cipher(b"0123456789abcdef");
    let data = encrypt(&mut cipher);
    println!("{:?}", data);
}

newpavlov · 2018-12-06T09:41:32Z

Any news on this?

I've updated Cargo.lock after merge, as it was the easiest way to merge.

sashahilton00 · 2019-03-15T16:27:22Z

Sorry, this PR kind of got lost to the winds. If you wouldn't mind rebasing this, we'll review it and hopefully get it merged.

newpavlov · 2019-03-15T17:00:09Z

No problem! I've rebased PR and additionally fixed one security related mistake.

newpavlov · 2019-03-15T17:20:35Z

core/src/authentication.rs

+            assert_eq!(data % block_size, 0);
+            for chunk in data.chunks_mut(block_size) {
+                cipher.decode(GenericArray::from_slice_mut(chunk));
+            }


I am not sure about this part. Can it be assumed that data here is always multiple of 16?

it doesn't look like it. I just checked with my stored blob, where the hex blob was 286 characters

Hm, this is quite strange. If ECB (or CBC) mode was used encrypted message length should be a multiple of block size. Do you have any test vectors for those encrypted blocks or implementations which can be used as a reference?

Will have to check later. I don't believe so, I believe we just store the bllob as we receive it, no telling what parameters are used in it's creation.

sashahilton00 · 2019-03-16T03:43:41Z

Thanks, will test this and review tomorrow hopefully. Also, could you squash the recent commits by any chance?

sashahilton00 · 2019-03-16T14:38:29Z

This seems to work fine. Once the last few commits are squashed will merge.

newpavlov · 2019-03-16T15:33:45Z

Done! Don't forget about checking decryption in Credentials::with_blob and ideally it would be nice to add some tests for it.

sashahilton00 · 2019-03-16T15:34:46Z

I authed using zeroconf, then restarted with cached credentials and it worked fine.

newpavlov added 2 commits July 23, 2018 16:41

replace rust-crypto's hashes, HMAC and PBKDF2

e467702

aes-ctr

1f1cd11

newpavlov changed the title ~~WIP: Replace rust-crypto crate with RustCrypto crates~~ Replace rust-crypto crate with RustCrypto crates Jul 30, 2018

bump min Rust version to 1.27

2ae5589

ashthespy mentioned this pull request Oct 13, 2018

UB error when running on aarch64 RustCrypto/stream-ciphers#5

Closed

newpavlov force-pushed the master branch from 23ae4a2 to 84ec6c8 Compare October 15, 2018 01:55

update dependencies

0626655

newpavlov force-pushed the master branch from 84ec6c8 to 0626655 Compare October 15, 2018 02:08

newpavlov added 2 commits December 6, 2018 09:38

Merge branch 'master' into master

7725915

update deps

5706562

newpavlov added 2 commits December 6, 2018 12:49

update aes-ctr to 0.3

4bd0fc8

update Cargo.lock

54974d8

newpavlov mentioned this pull request Jan 19, 2019

Replace rust-crypto with sha-1 crate Spotifyd/spotifyd#199

Merged

Merge branch 'master' into master

391b9c6

newpavlov commented Mar 15, 2019

View reviewed changes

update Cargo.lock, use constant time equality check, remove block-modes

7258944

newpavlov force-pushed the master branch from 5ac00fd to 7258944 Compare March 16, 2019 15:30

sashahilton00 merged commit 308ff32 into librespot-org:master Mar 20, 2019

mfrischknecht mentioned this pull request Jan 14, 2021

slimserver fails to communicate with Spotify on Raspberry Pi 4 NixOS/nixpkgs#109270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace rust-crypto crate with RustCrypto crates #239

Replace rust-crypto crate with RustCrypto crates #239

newpavlov commented Jul 23, 2018

newpavlov commented Jul 30, 2018

ashthespy commented Sep 23, 2018

ashthespy commented Oct 12, 2018

newpavlov commented Oct 12, 2018 •

edited

ashthespy commented Oct 12, 2018 •

edited

ashthespy commented Oct 12, 2018 •

edited

newpavlov commented Oct 12, 2018

newpavlov commented Oct 12, 2018

ashthespy commented Oct 12, 2018 •

edited

ashthespy commented Oct 13, 2018

newpavlov commented Oct 13, 2018

ashthespy commented Oct 13, 2018

newpavlov commented Oct 13, 2018

ashthespy commented Oct 13, 2018 •

edited

newpavlov commented Oct 13, 2018

newpavlov commented Dec 6, 2018

sashahilton00 commented Mar 15, 2019

newpavlov commented Mar 15, 2019

newpavlov Mar 15, 2019

sashahilton00 Mar 15, 2019

newpavlov Mar 15, 2019

sashahilton00 Mar 15, 2019

sashahilton00 commented Mar 16, 2019

sashahilton00 commented Mar 16, 2019

newpavlov commented Mar 16, 2019 •

edited

sashahilton00 commented Mar 16, 2019

Replace rust-crypto crate with RustCrypto crates #239

Replace rust-crypto crate with RustCrypto crates #239

Conversation

newpavlov commented Jul 23, 2018

newpavlov commented Jul 30, 2018

ashthespy commented Sep 23, 2018

ashthespy commented Oct 12, 2018

newpavlov commented Oct 12, 2018 • edited

ashthespy commented Oct 12, 2018 • edited

ashthespy commented Oct 12, 2018 • edited

newpavlov commented Oct 12, 2018

newpavlov commented Oct 12, 2018

ashthespy commented Oct 12, 2018 • edited

ashthespy commented Oct 13, 2018

newpavlov commented Oct 13, 2018

ashthespy commented Oct 13, 2018

newpavlov commented Oct 13, 2018

ashthespy commented Oct 13, 2018 • edited

newpavlov commented Oct 13, 2018

newpavlov commented Dec 6, 2018

sashahilton00 commented Mar 15, 2019

newpavlov commented Mar 15, 2019

newpavlov Mar 15, 2019

Choose a reason for hiding this comment

sashahilton00 Mar 15, 2019

Choose a reason for hiding this comment

newpavlov Mar 15, 2019

Choose a reason for hiding this comment

sashahilton00 Mar 15, 2019

Choose a reason for hiding this comment

sashahilton00 commented Mar 16, 2019

sashahilton00 commented Mar 16, 2019

newpavlov commented Mar 16, 2019 • edited

sashahilton00 commented Mar 16, 2019

newpavlov commented Oct 12, 2018 •

edited

ashthespy commented Oct 12, 2018 •

edited

ashthespy commented Oct 12, 2018 •

edited

ashthespy commented Oct 12, 2018 •

edited

ashthespy commented Oct 13, 2018 •

edited

newpavlov commented Mar 16, 2019 •

edited