Memory leak in HTTP Digest Authentication (IDFGH-14071)

[7aman]

esp-idf/components/esp_http_client/esp_http_client.c

Line 641 in 7a305c0

client->auth_data->uri = NULL;

I believe the URI should be freed before being set to null. When I try to dump it in previous line, it still contains the URI value.

Try this simple example that sends a GET request 100 times in a loop.
Since the URI consists of the path and query, longer values could exacerbate the leak.

#define MAX_RETRY (100)

static esp_err_t simple_event_handler(esp_http_client_event_t* evt)
{
    switch (evt->event_id) {
    case HTTP_EVENT_ON_DATA:
        printf("recv> [len:%d]%.*s\n", evt->data_len, evt->data_len, (const char*)evt->data);
        break;

    default:
        break;
    }
    return ESP_OK;
}

static void example_task(void* p)
{
    printf("starting...\n");
    for (int i = 0; i < MAX_RETRY; ++i) {
        esp_http_client_config_t config = {
            .host = "YOUR_LOCAL_IP",
            .port = 8000,
            .path = "/a/very/long/path/can/exacerbate/the/leak",
            .query = "key=a-very-long-value-could-exacerbate-the-leak",
            .buffer_size_tx = 4096,
            .auth_type = HTTP_AUTH_TYPE_DIGEST,
            .username = "admin",
            .password = "admin",
            .event_handler = simple_event_handler,
        };
        esp_http_client_handle_t client = esp_http_client_init(&config);
        esp_http_client_set_header(client, "Upgrade-Insecure-Requests", "1");
        esp_http_client_perform(client);
        esp_http_client_cleanup(client);
        vTaskDelay(pdMS_TO_TICKS(1000));
    }
    printf("done.\n");
    while (1) {
        vTaskDelay(pdMS_TO_TICKS(1000));
    }
    vTaskDelete(NULL);
}

Here's a simple Python server that listens on port 8000 and responds with "Hello" to all incoming requests:

from http.server import HTTPServer, BaseHTTPRequestHandler
import hashlib
import os
import re

# Define the username, password, realm, and quality of protection
USERNAME = "admin"
PASSWORD = "admin"
REALM = "Simple Digest Auth"
NONCE = os.urandom(16).hex()  # Fixed nonce for simplicity in this example
QOP = "auth"                  # Define qop as "auth" for quality of protection

# The response message to send to authenticated clients
RESPONSE_MESSAGE = "hello"

def generate_digest_response(username, realm, password, method, uri, nonce, nc, cnonce, qop):
    """Generate the expected Digest response for comparison."""
    # HA1: Hash of username:realm:password
    ha1 = hashlib.md5(f"{username}:{realm}:{password}".encode()).hexdigest()
    # HA2: Hash of method:uri
    ha2 = hashlib.md5(f"{method}:{uri}".encode()).hexdigest()
    # Response: Hash of HA1:nonce:nc:cnonce:qop:HA2
    response = hashlib.md5(f"{ha1}:{nonce}:{nc}:{cnonce}:{qop}:{ha2}".encode()).hexdigest()
    return response

def parse_digest_header(header):
    """Parse the Digest Authorization header into a dictionary."""
    # Regular expression to match key="value" pairs
    pattern = re.compile(r'(\b\w+\b)="?([^",]+)"?')
    return dict(pattern.findall(header))

class DigestAuthHandler(BaseHTTPRequestHandler):
    def do_GET(self):
        # Parse the Authorization header for Digest Authentication
        auth_header = self.headers.get('Authorization')
        
        if auth_header is None or not auth_header.startswith('Digest '):
            self.request_authentication()
            return

        # Parse the digest values using the parse_digest_header function
        auth_params = parse_digest_header(auth_header[7:])

        # Validate the username, realm, nonce, uri, qop, nc, cnonce, and response
        if (auth_params.get("username") == USERNAME and
            auth_params.get("realm") == REALM and
            auth_params.get("nonce") == NONCE and
            auth_params.get("uri") == self.path and
            auth_params.get("qop") == QOP):
            
            # Compute the expected response using the qop, nc, and cnonce values
            expected_response = generate_digest_response(
                USERNAME, REALM, PASSWORD, self.command, self.path,
                NONCE, auth_params.get("nc"), auth_params.get("cnonce"), QOP
            )
            
            # If the response matches, grant access
            if auth_params.get("response") == expected_response:
                # Respond with the specified message and set Content-Length header
                self.send_response(200)
                self.send_header("Content-type", "text/plain")
                self.send_header("Content-Length", str(len(RESPONSE_MESSAGE)))
                self.end_headers()
                self.wfile.write(RESPONSE_MESSAGE.encode())
                return

        # If authentication fails, request it again
        self.request_authentication()

    def request_authentication(self):
        """Request Digest Authentication from the client with qop="auth", without returning a payload."""
        self.send_response(401)
        self.send_header(
            'WWW-Authenticate',
            f'Digest realm="{REALM}", nonce="{NONCE}", qop="{QOP}", algorithm="MD5"'
        )
        self.send_header('Content-type', 'text/plain')
        self.end_headers()  # No payload is written to the response body

# Define the server's address and port
server_address = ('', 8000)
httpd = HTTPServer(server_address, DigestAuthHandler)

print("Server running on port 8000...")
httpd.serve_forever()

[7aman]

Could it be fixed in the next stable release?

[X-Ryl669]

The code does this:

#ifdef CONFIG_ESP_HTTP_CLIENT_ENABLE_DIGEST_AUTH
        } else if (client->connection_info.auth_type == HTTP_AUTH_TYPE_DIGEST && client->auth_data) {
            client->auth_data->uri = NULL;
            http_utils_assign_string(&client->auth_data->uri,client->connection_info.path,-1);
            if (client->connection_info.query){
                http_utils_append_string(&client->auth_data->uri,"?",-1);
                http_utils_append_string(&client->auth_data->uri,client->connection_info.query,-1);
            }
            client->auth_data->cnonce = ((uint64_t)esp_random() << 32) + esp_random();
            auth_response = http_auth_digest(client->connection_info.username, client->connection_info.password, client->auth_data);
#endif

If you look at the allocation of client->auth_data, it's calloced above, uri member isn't. So the line client->auth_data->uri = NULL; is correct and required, since http_utils_append_string will check of this value and if null, allocate for the given part (else, it realloc).

Yet, the client freeing code does free the uri (see here) so as long as the client isn't reused, there isn't any possible memory leak.

Thus the order of operations is:

1. client allocate auth_data here
2. auth_data->uri is zero/NULL by default, since the code is using calloc (not malloc)
3. When using DIGEST authorization (in prepare), the URI is allocated/reallocated as required to store the URI for the digest protocol
4. Then you perform the request
5. Then you clean the client and the URI is freed

In you example code, you're not calling the esp_http_client_prepare so the URI buffer isn't allocated.

However, you're right that there's a potential leak path in that case, since the server will likely answer with an "401 Unauthorized" answer, leading to this line so that the perform will prepare again here and in turn this will call prepare again, leading to leaking the URI buffer.

So, there are 2 possible fix up for this leak, either replace line 641 to read:

free(client->auth_data->uri); 
client->auth_data->uri = NULL;

since it's ok to call free on a NULL pointer.

or, better (to avoid fragmenting the heap), check if it's required to update the URI if one exists and reallocate only if required. This can be done easily like this:

if (client->auth_data->uri 
&& strlen(client->auth_data->uri) > (strlen(client->connection_info.path) + client->connection_info.query? strlen(client->connection_info.query) + 1 : 0)) { 
    free(client->auth_data->uri); 
    client->auth_data->uri = NULL; 
}

So meanwhile, you can simply call esp_http_client_prepare before perform, so you won't get a redirect and the prepare will only be called once and not leak.

[7aman]

Thus the order of operations is:

1. client allocate `auth_data` [here](https://github.com/espressif/esp-idf/blob/7a305c0284b7af7cd8b8f12b48f72e2685d9a363/components/esp_http_client/esp_http_client.c#L710)

2. auth_data->uri is zero/NULL by default, since the code is using calloc (not malloc)

3. When using DIGEST authorization (in prepare), the URI is allocated/reallocated as required to store the URI for the digest protocol

4. Then you perform the request

5. Then you clean the client and the URI is freed

In you example code, you're not calling the esp_http_client_prepare so the URI buffer isn't allocated.

So meanwhile, you can simply call esp_http_client_prepare before perform, so you won't get a redirect and the prepare will only be called once and not leak.

Thank you for your response.

It seems that esp_http_client_prepare is a static function and cannot be directly called from user code.

From my understanding of HTTP Digest Authentication, the process requires sending an initial request to the server to receive certain parameters (like the nonce). These parameters are then used to compute a proper response, which is included in a second request to authenticate successfully. Could you clarify where the preparation step for this process would typically occur in the code?

[X-Ryl669]

You're right, I didn't check visibility of the method.

In Digest authorization, either the client or the server can generate the nonce, so you don't need 2 actual requests per query. Once you know the realm (this requires an initial request), you can skip querying the nonce for later request.

Digest authorization is being shifted out toward basic authentication over HTTPS which is, paradoxically, more secure than using MD5 for hashing/protecting the password.

Anyway, this means that the only official way to create the right header is to go by redirections (401) like you did and then pay the price of an header memory leak as you've spotted. So use either fix I've listed to plug this leak.