ossrs / srs

SRS is a simple, high-efficiency, real-time media server supporting RTMP, WebRTC, HLS, HTTP-FLV, HTTP-TS, SRT, MPEG-DASH, and GB28181.
https://ossrs.io
MIT License
25.46k stars 5.35k forks source link

Potential NULL dereference in srs_app_rtc_dtls.cpp #3833

Closed icy17 closed 6 months ago

icy17 commented 11 months ago

Describe the bug In trunk/src/app/srs_app_rtc_dtls.cpp: 159, calling SSL_CTX_set_cipher_list without checking the parameter 1 might cause a null dereference.

Version master

Expected behavior It's better to add a check before using the pointer.

winlinvip commented 11 months ago

The dtls_ctx was created earlier.

    SSL_CTX* dtls_ctx;
#if OPENSSL_VERSION_NUMBER < 0x10002000L // v1.0.2
    dtls_ctx = SSL_CTX_new(DTLSv1_method());
#else
    if (version == SrsDtlsVersion1_0) {
        if (role == "active") {
            dtls_ctx = SSL_CTX_new(DTLSv1_client_method());
        } else {
            dtls_ctx = SSL_CTX_new(DTLSv1_server_method());
        }
    } else if (version == SrsDtlsVersion1_2) {
        if (role == "active") {
            dtls_ctx = SSL_CTX_new(DTLS_client_method());
        } else {
            dtls_ctx = SSL_CTX_new(DTLS_server_method());
        }
    } else {
        // SrsDtlsVersionAuto, use version-flexible DTLS methods
        dtls_ctx = SSL_CTX_new(DTLS_method());
    }
#endif

If the dtls_ctx creation fails, it's a serious error, usually due to insufficient memory. If the allocation fails, there will be a null pointer when it's used later. If we have to check for null every time, it would make the code very hard to maintain. Instead, for these kinds of irrecoverable errors, we generally use assert.

    SSL_CTX* dtls_ctx;
#if OPENSSL_VERSION_NUMBER < 0x10002000L // v1.0.2
    dtls_ctx = SSL_CTX_new(DTLSv1_method());
#endif
    srs_assert(dtls_ctx);

This way, we don't have to check for null all the time, which makes the code much easier to read and maintain.

Feel free to submit a patch.

TRANS_BY_GPT4