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

Fix bug for upgrading to OpenSSL 3.0. v5.0.189 v6.0.89 #3827

Merged

Conversation

duiniuluantanqin
Copy link
Member

@duiniuluantanqin duiniuluantanqin commented Oct 8, 2023

The fix is for the DH_set_length error. As shown in lines 2-5, OpenSSL 3.0 added a check for length, which allowed this issue to be exposed.

1 if (dh->params.q == NULL) {
2       /* secret exponent length, must satisfy 2^(l-1) <= p */
3        if (dh->length != 0
4            && dh->length >= BN_num_bits(dh->params.p))
5            goto err;
6        l = dh->length ? dh->length : BN_num_bits(dh->params.p) - 1;
7        if (!BN_priv_rand_ex(priv_key, l, BN_RAND_TOP_ONE,
8                             BN_RAND_BOTTOM_ANY, 0, ctx))
9            goto err;
        ... ...
    }

Co-authored-by: john hondaxiao@tencent.com

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Oct 8, 2023
@duiniuluantanqin duiniuluantanqin changed the title adapt openssl 3.0 adapt to openssl 3.0 Oct 9, 2023
@winlinvip
Copy link
Member

winlinvip commented Oct 10, 2023

When we used this setting to create a 128-byte key length, it was during the RTMP handshake to generate a 128-byte shared key. But in reality, it's not necessary to call this function.

Also, during the RTMP handshake, we should be getting the shared key, but there seems to be a bit of an issue with the SRS logic, as it directly uses the private key. Of course, since the RTMP handshake doesn't verify the key, it doesn't really matter which key we use.

In addition, RTMP checks the length of the key. If it's not 128, like if it accidentally becomes 127 bytes, it will generate it again. This issue has been noticed in reality, but this logic isn't necessary. As long as the result is a 128-byte key, it's fine.

Basically, RTMP clients don't usually check the 128-byte key data during the handshake process. So, this background might not be clear, hence the special note for clarification.

TRANS_BY_GPT4

@winlinvip winlinvip changed the title adapt to openssl 3.0 Fix bug for upgrading OpenSSL to 3.0 Oct 10, 2023
@winlinvip
Copy link
Member

winlinvip commented Oct 10, 2023

We need to merge into 5 and 6.

TRANS_BY_GPT4

@winlinvip winlinvip changed the title Fix bug for upgrading OpenSSL to 3.0 Fix bug for upgrading to OpenSSL 3.0 Oct 10, 2023
@xiaozhihong xiaozhihong changed the title Fix bug for upgrading to OpenSSL 3.0 Fix bug for upgrading to OpenSSL 3.0. v5.0.189 v6.0.89 Oct 11, 2023
@xiaozhihong xiaozhihong added the RefinedByAI Refined by AI/GPT. label Oct 11, 2023
@xiaozhihong xiaozhihong merged commit 0649a6d into ossrs:develop Oct 11, 2023
17 checks passed
xiaozhihong added a commit that referenced this pull request Oct 11, 2023
The fix is for the DH_set_length error. As shown in lines 2-5, OpenSSL
3.0 added a check for length, which allowed this issue to be exposed.
```
1 if (dh->params.q == NULL) {
2       /* secret exponent length, must satisfy 2^(l-1) <= p */
3        if (dh->length != 0
4            && dh->length >= BN_num_bits(dh->params.p))
5            goto err;
6        l = dh->length ? dh->length : BN_num_bits(dh->params.p) - 1;
7        if (!BN_priv_rand_ex(priv_key, l, BN_RAND_TOP_ONE,
8                             BN_RAND_BOTTOM_ANY, 0, ctx))
9            goto err;
        ... ...
    }
```

---------

Co-authored-by: john <hondaxiao@tencent.com>
duiniuluantanqin pushed a commit to duiniuluantanqin/srs that referenced this pull request Oct 11, 2023
The fix is for the DH_set_length error. As shown in lines 2-5, OpenSSL
3.0 added a check for length, which allowed this issue to be exposed.
```
1 if (dh->params.q == NULL) {
2       /* secret exponent length, must satisfy 2^(l-1) <= p */
3        if (dh->length != 0
4            && dh->length >= BN_num_bits(dh->params.p))
5            goto err;
6        l = dh->length ? dh->length : BN_num_bits(dh->params.p) - 1;
7        if (!BN_priv_rand_ex(priv_key, l, BN_RAND_TOP_ONE,
8                             BN_RAND_BOTTOM_ANY, 0, ctx))
9            goto err;
        ... ...
    }
```

---------

Co-authored-by: john <hondaxiao@tencent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. RefinedByAI Refined by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants