OpenSSL occasionally printing random garbage

user_13262787 0
Viewed 92

I have just started using OpenSSL in order to make HTTPS requests. I'm using C++. Sometimes, I get a proper response which looks something like this:

HTTP/1.1 200 OK
Server: nginx/1.16.1
Date: Sun, 09 Aug 2020 12:14:03 GMT
Content-Type: application/json; charset=UTF-8
Content-Length: 159
Connection: keep-alive
Access-Control-Allow-Credentials: true
Vary: Origin

{"status":"success","symbol":"AAPL","last":{"price":446.71,"size":200,"exchange":2,"cond1":14,"cond2":12,"cond3":41,"cond4":0,"timestamp":1596835355456000000}}

and sometimes I get a problematic response which looks something like this: (scroll to the right)

HTTP/1.1 200 OK
Server: nginx/1.16.1
Date: Sun, 09 Aug 2020 12:14:06 GMT
Content-Type: application/json; charset=UTF-8
Content-Length: 159
Connection: keep-alive
Access-Control-Allow-Credentials: true
Vary: Origin

{"status":"success","symbol":"AAPL","last":{"price":446.71,"size":200,"exchange":2,"cond1":14,"cond2":12,"cond3":41,"cond4":0,"timestamp":15968353554560000?????`9??????p???????p?#???????`???????u???????@???????U?????? ??????5??????

I think the issue has something to do with the SSL_read function adding things to the buffer, but I am unsure. I know this issue isn't specific to the website since I have tried making requests to other websites and I got the same result. Here is the code for the request method:

Response* Requests::get(const char* ip_addr, std::string& route, datastructures::hash::HashTable& headers, datastructures::hash::HashTable& parameters){
    int sock = socket(AF_INET, SOCK_STREAM, 0);
    struct sockaddr_in server;
    server.sin_port = htons(443);
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = inet_addr(ip_addr);
    connect(sock, (struct sockaddr*)&server, sizeof(server));
    cout<<SSL_load_error_strings()<<endl;  
    cout<<SSL_library_init()<<endl; 
    SSL_CTX* ctx = SSL_CTX_new(SSLv23_client_method()); 
    SSL* conn = SSL_new(ctx);
    cout<<SSL_set_fd(conn, sock)<<endl; 
    cout<<SSL_connect(conn)<<endl; 
    std::vector<datastructures::hash::Key> params = parameters.getTable();
    std::vector<datastructures::hash::Key> heads = headers.getTable();
    std::ostringstream os;
    os << "GET " << route <<"?";
    std::string request = os.str();
    os.str("");
    {
      int i;
      for(i=0;i<params.size();i++){
        os << params[i].key << "="<< *(std::string*)(params[i].value) << "&";
        request+=os.str();
        os.str("");
      }
      request += " HTTP/1.1 \r\n";
      request+=os.str();
      for(i=0;i<heads.size();i++){
        os << heads[i].key << ": " << *(std::string*)(heads[i].value) << "\r\n";
        request+=os.str();
        os.str("");
      }
      request += "\r\n";
    }
    SSL_write(conn, (const char*)request.c_str(), request.size());
    std::string ret;
    const char* retc = (const char*)malloc(10240*sizeof(char));
    int recv = SSL_read(conn, (void *)retc, 10240);
    while(true){
      ret+=retc;
      free((char*)retc);
      if(recv < 10240){
        break;
      } else{
        recv = SSL_read(conn, (void *)retc, 10240);
        retc = (const char*)malloc(10240*sizeof(char));
      }
    }
    SSL_free(conn);
    close(sock);
    return new Response(ret);

and here is how I call the method in my main function:

  std::cout << requests::Requests::get("35.186.171.205", route, headers, parameters)->getRawResponse() << std::endl; 
//Get raw response is a method in my response class that just returns the raw http response
openssl https c c++
1 Answers

There are multiple bugs in the shown code.

ret+=retc;

This only works, when retc is a plain char *, when the string it points to is terminated by a '\0'. SSL_read does not do anything of that sort. You should use std::string's append() overload that takes a pointer and an exact number of character to add to this string.

  free((char*)retc);

  // ...

    recv = SSL_read(conn, (void *)retc, 10240);
    retc = (const char*)malloc(10240*sizeof(char));

This sequence of code:

  1. Deallocates retc. This pointer is now invalid.

  2. Attempts to read something into retc, which is no longer valid.

  3. Allocates a new retc pointer.

A new buffer should be allocated before reading something into it. Furthermore, it is not even necessary to do all of this work in the first place. The buffer that was previously used to SSL_read something is perfectly usable to SSL_read something more. It is perfectly adequate for the job. freeing a buffer, just to malloc it again, accomplishes absolutely nothing useful, whatsoever.

Finally, if you really intend to write modern C++ code, there is no need to malloc or free anything. Just use a std::vector<char> and have the vector, itself, handle all the memory allocation and deallocation for you. You rarely see manual memory allocation and deallocation in modern C++ code, which uses containers to properly handle and keep track of all memory correctly, and automatically cleans it up for you avoiding memory leaks.

Just replace all your instances of manually allocating memory with a std::vector<char>.

user_39433120
Related