Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to cryptographically strong PRNG #102

Closed
gavv opened this issue Aug 6, 2017 · 20 comments
Closed

Switch to cryptographically strong PRNG #102

gavv opened this issue Aug 6, 2017 · 20 comments
Assignees
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project enhancement help wanted An important and awaited task but we have no human resources for it yet security Security, encryption
Milestone

Comments

@gavv
Copy link
Member

gavv commented Aug 6, 2017

Problem

In RTP, the SSRC value and the initial value for seqnums and timestamps should be random.

Here is what RFC 3550 says about seqnums:

The initial value of the sequence number
SHOULD be random (unpredictable) to make known-plaintext attacks
on encryption more difficult, even if the source itself does not
encrypt according to the method in Section 9.1, because the
packets may flow through a translator that does. Techniques for
choosing unpredictable numbers are discussed in [17].

These requirements have security reasons. The RFC recommends using a cryptographically strong PRNG (CSPRNG), but currently our PRNG is uniform but not cryptographically strong.

Solution

We already use libuv, which also provides CSPRNG: http://docs.libuv.org/en/v1.x/misc.html#c.uv_random

Here is what we should do:

  • Rename current PRNG: roc_core/target_posix/roc_core/random.h|cpp to roc_core/target_posix/roc_core/fast_random.h|cpp and core::random() to core::fast_random().

  • Add roc_core/target_libuv/roc_core/secure_random.h|cpp with core::secure_random(). Implement is using synchronous version of uv_random() (set loop, req, and cb to NULL).

  • Use secure_random() instead of fast_random() in audio::Packetizer and fec::Writer.

@gavv gavv added the defect Something isn't working label Aug 6, 2017
@gavv gavv added this to Backlog in kanban board Apr 23, 2019
@gavv gavv added enhancement and removed defect Something isn't working labels Apr 23, 2019
@gavv gavv changed the title Cryptographic secure PRNG Cryptographically secure PRNG May 31, 2019
@gavv gavv added help wanted An important and awaited task but we have no human resources for it yet easy hacks The solution is expected to be straightforward even if you are new to the project good for newcomers labels Jul 5, 2019
@gavv gavv removed easy hacks The solution is expected to be straightforward even if you are new to the project good first issue labels Jul 31, 2019
@gavv gavv moved this from Backlog to Help wanted in kanban board Sep 11, 2019
@gavv gavv added the easy hacks The solution is expected to be straightforward even if you are new to the project label Dec 2, 2019
@gavv gavv changed the title Cryptographically secure PRNG Use cryptographically strong PRNG Dec 2, 2019
@gavv gavv changed the title Use cryptographically strong PRNG Switch to cryptographically strong PRNG Dec 4, 2019
@fusuiyi123
Copy link
Contributor

Hi @gavv i will send a pr for this, thanks.

@fusuiyi123
Copy link
Contributor

Q: I don't quite understand the original random() usage here: https://github.com/roc-project/roc/blob/master/src/modules/roc_audio/packetizer.cpp#L38, and int uv_random(uv_loop_t* loop, uv_random_t* req, void* buf, size_t buflen, unsigned int flags, uv_random_cb cb) returns succeed or not, which I don't quite understand how to use it here. New to this repo and your docs really impressed me, Would appreciate your help.

@gavv
Copy link
Member Author

gavv commented Mar 27, 2020

@fusuiyi123 Hi, you're welcome.

I don't quite understand the original random() usage here

We generate a random number in range [0; 2^32) and assign it to source_. (packet::source_t is a 32-bit unsigned integer and so packet::source_t(-1) is 2^32-1).

int uv_random(...) returns succeed or not, which I don't quite understand how to use it here.

If uv_random fails, we should fail Packetizer initialization. We can do it by adding bool valid_ field and using it like this:

Packetizer::Packetizer() 
    : valid_(false) {
    if (something failed) {
        return;
    }
    valid_ = true;
}

bool Packetizer::valid() const {
    return valid_;
}

Then we can use Packetizer::valid() to check whether it was initialized correctly, somewhere in roc_pipeline when it's created. We use the same approach for many other classes, just grep for valid_ string.

@fusuiyi123
Copy link
Contributor

Hi @gavv sorry if I ask dumb question.
Here

    , source_((packet::source_t)core::random(packet::source_t(-1)))
    , seqnum_((packet::seqnum_t)core::random(packet::seqnum_t(-1)))
    , timestamp_((packet::timestamp_t)core::random(packet::timestamp_t(-1))) {

these 3 var are assigned random number. Now we want to replace them with int uv_random(uv_loop_t* loop, uv_random_t* req, void* buf, size_t buflen, unsigned int flags, uv_random_cb cb), which however is Fill buf with exactly buflen cryptographically strong random bytes acquired from the system CSPRNG. How this can be used here? Thank you:)

@gavv
Copy link
Member Author

gavv commented Mar 28, 2020

You can look at current core::random() implementation (which we should rename to core::fast_random()). I think we need something similar, but using uv_random() instead of nrand48() to generate random numbers.

@fusuiyi123
Copy link
Contributor

hmm I am new to this concept, how to transform bytes void* buf to a random number..?

@gavv
Copy link
Member Author

gavv commented Mar 29, 2020

uv_random fills the provided buffer with random bytes. You can just use the number as "a buffer", something like this:

uint32_t num;
uv_random(NULL, NULL, &num, sizeof(num), 0, NULL);

After this call, num will contain a random integer in range [0; 2^32).

The next step is to generate a uniformly-distributed random numbers in range [from; to] based on this. We can use the same technique with a loop, as we do in current core::random() implementation.

To be honest, that's a bit strange question to me. If you feel new to C or C++, I can recommend you a few good books on them.

@fusuiyi123
Copy link
Contributor

thanks I finally understand it.. please recommend me a c++ book:)

@gavv
Copy link
Member Author

gavv commented Mar 29, 2020

please recommend me a c++ book:)

I've sent you an email.

@fusuiyi123
Copy link
Contributor

thanks! I met an issue: though I have #include <uv.h> in secure_random.cpp, I have src/modules/roc_core/target_posix/roc_core/secure_random.cpp:56:13: error: use of undeclared identifier 'uv_random' if (uv_random(NULL, NULL, &val, sizeof(val), 0, NULL) && val >= min) {

@fusuiyi123
Copy link
Contributor

I saw this is New in version 1.33.0. and https://github.com/roc-project/roc/blob/master/SConstruct#L28 gives 1.5.0 default, I am not sure if it is related to this

@gavv
Copy link
Member Author

gavv commented Mar 29, 2020

Oh, I didn't notice that this is a new function. So you're using an older libuv?

Then let's do the following:

  • change default from 1.5.0 to 1.33.0
  • add in ifdef to check libuv version; if we're using an old one, fallback to fast_random()

@gavv
Copy link
Member Author

gavv commented Mar 29, 2020

Later we'll add an optional alternative secure_random() implementation using OpenSSL, so it'll be possible to have a correct implementation even with old libuv.

@fusuiyi123
Copy link
Contributor

Thanks so much, I raised a pr and listed my question over there.

@gavv
Copy link
Member Author

gavv commented Mar 30, 2020

FYI: 90e5533 (pushed to develop)

gavv pushed a commit that referenced this issue Apr 6, 2020
@gavv gavv added this to the 0.2.0 milestone Apr 6, 2020
@gavv
Copy link
Member Author

gavv commented Apr 6, 2020

PR is merged.

@gavv gavv closed this as completed Apr 6, 2020
kanban board automation moved this from Help wanted to Done Apr 6, 2020
gavv pushed a commit that referenced this issue Apr 29, 2020
gavv pushed a commit that referenced this issue May 1, 2020
gavv pushed a commit that referenced this issue May 18, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue May 21, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue May 21, 2020
@gavv gavv mentioned this issue May 24, 2020
11 tasks
@gavv gavv added security Security, encryption enhancement and removed enhancement labels May 24, 2020
@gavv
Copy link
Member Author

gavv commented May 24, 2020

@fusuiyi123 FYI: #364

gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jun 5, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jun 7, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jun 10, 2020
gavv pushed a commit that referenced this issue Jun 27, 2020
gavv pushed a commit that referenced this issue Jun 27, 2020
gavv pushed a commit that referenced this issue Jun 28, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jul 2, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jul 2, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Jul 7, 2020
gavv pushed a commit that referenced this issue Jul 7, 2020
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Mar 5, 2021
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Mar 5, 2021
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Nov 29, 2022
gavv pushed a commit to gavv/roc-toolkit that referenced this issue Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project enhancement help wanted An important and awaited task but we have no human resources for it yet security Security, encryption
Projects
Development

Successfully merging a pull request may close this issue.

2 participants