PACKET: add PACKET_memdup and PACKET_strndup

Use each once in s3_srvr.c to show how they work.

Also fix a bug introduced in c3fc7eea
and made apparent by this change:
ssl3_get_next_proto wasn't updating next_proto_negotiated_len
Reviewed-by: NMatt Caswell <matt@openssl.org>

ssl/packet_locl.h

@@ -335,9 +335,12 @@ __owur static inline int PACKET_peek_copy_bytes(const PACKET *pkt,
     return 1;
 }
 
-/* Read |len| bytes from |pkt| and copy them to |data| */
+/*
+ * Read |len| bytes from |pkt| and copy them to |data|.
+ * The caller is responsible for ensuring that |data| can hold |len| bytes.
+ */
 __owur static inline int PACKET_copy_bytes(PACKET *pkt, unsigned char *data,
-                                          size_t len)
+                                           size_t len)
 {
     if (!PACKET_peek_copy_bytes(pkt, data, len))
         return 0;
@@ -347,6 +350,55 @@ __owur static inline int PACKET_copy_bytes(PACKET *pkt, unsigned char *data,
     return 1;
 }
 
+/*
+ * Copy |pkt| bytes to a newly allocated buffer and store a pointer to the
+ * result in |*data|, and the length in |len|.
+ * If |*data| is not NULL, the old data is OPENSSL_free'd.
+ * If the packet is empty, or malloc fails, |*data| will be set to NULL.
+ * Returns 1 if the malloc succeeds and 0 otherwise.
+ * Does not forward PACKET position (because it is typically the last thing
+ * done with a given PACKET).
+ */
+__owur static inline int PACKET_memdup(const PACKET *pkt, unsigned char **data,
+                                       size_t *len)
+{
+    size_t length;
+
+    OPENSSL_free(*data);
+    *data = NULL;
+    *len = 0;
+
+    length = PACKET_remaining(pkt);
+
+    if (length == 0)
+        return 1;
+
+    *data = BUF_memdup(pkt->curr, length);
+
+    if (*data == NULL)
+        return 0;
+
+    *len = length;
+    return 1;
+}
+
+/*
+ * Read a C string from |pkt| and copy to a newly allocated, NUL-terminated
+ * buffer. Store a pointer to the result in |*data|.
+ * If |*data| is not NULL, the old data is OPENSSL_free'd.
+ * If the data in |pkt| does not contain a NUL-byte, the entire data is
+ * copied and NUL-terminated.
+ * Returns 1 if the malloc succeeds and 0 otherwise.
+ * Does not forward PACKET position (because it is typically the last thing done
+ * with a given PACKET).
+ */
+__owur static inline int PACKET_strndup(const PACKET *pkt, char **data)
+{
+    OPENSSL_free(*data);
+    *data = BUF_strndup((const char*)pkt->curr, PACKET_remaining(pkt));
+    return (*data != NULL);
+}
+
 /* Move the current reading position back |len| bytes */
 __owur static inline int PACKET_back(PACKET *pkt, size_t len)
 {

ssl/s3_srvr.c

@@ -2250,13 +2250,14 @@ int ssl3_get_client_key_exchange(SSL *s)
     if (alg_k & SSL_PSK) {
         unsigned char psk[PSK_MAX_PSK_LEN];
         size_t psklen;
+	PACKET psk_identity;
 
-        if (!PACKET_get_net_2(&pkt, &i)) {
+        if (!PACKET_get_length_prefixed_2(&pkt, &psk_identity)) {
             al = SSL_AD_DECODE_ERROR;
             SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
             goto f_err;
         }
-        if (i > PSK_MAX_IDENTITY_LEN) {
+        if (PACKET_remaining(&psk_identity) > PSK_MAX_IDENTITY_LEN) {
             al = SSL_AD_DECODE_ERROR;
             SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
                    SSL_R_DATA_LENGTH_TOO_LONG);
@@ -2269,21 +2270,10 @@ int ssl3_get_client_key_exchange(SSL *s)
             goto f_err;
         }
 
-        OPENSSL_free(s->session->psk_identity);
-        s->session->psk_identity = OPENSSL_malloc(i + 1);
-        if (s->session->psk_identity == NULL) {
+        if (!PACKET_strndup(&psk_identity, &s->session->psk_identity)) {
             al = SSL_AD_INTERNAL_ERROR;
-            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
-                   ERR_R_MALLOC_FAILURE);
-            goto f_err;
-        }
-        if (!PACKET_copy_bytes(&pkt, (unsigned char *)s->session->psk_identity,
-                               i)) {
-            al = SSL_AD_DECODE_ERROR;
-            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH);
             goto f_err;
         }
-        s->session->psk_identity[i] = '\0';
 
         psklen = s->psk_server_callback(s, s->session->psk_identity,
                                          psk, sizeof(psk));
@@ -3455,9 +3445,9 @@ int ssl3_send_cert_status(SSL *s)
 int ssl3_get_next_proto(SSL *s)
 {
     int ok;
-    unsigned int proto_len, padding_len;
     long n;
-    PACKET pkt;
+    PACKET pkt, next_proto, padding;
+    size_t next_proto_len;
 
     /*
      * Clients cannot send a NextProtocol message if we didn't see the
@@ -3506,25 +3496,20 @@ int ssl3_get_next_proto(SSL *s)
      *   uint8 padding_len;
      *   uint8 padding[padding_len];
      */
-    if (!PACKET_get_1(&pkt, &proto_len)){
+    if (!PACKET_get_length_prefixed_1(&pkt, &next_proto)
+        || !PACKET_get_length_prefixed_1(&pkt, &padding)
+        || PACKET_remaining(&pkt) > 0) {
         SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, SSL_R_LENGTH_MISMATCH);
         goto err;
     }
 
-    s->next_proto_negotiated = OPENSSL_malloc(proto_len);
-    if (s->next_proto_negotiated == NULL) {
-        SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, ERR_R_MALLOC_FAILURE);
+    if (!PACKET_memdup(&next_proto, &s->next_proto_negotiated,
+                       &next_proto_len)) {
+        s->next_proto_negotiated_len = 0;
         goto err;
     }
 
-    if (!PACKET_copy_bytes(&pkt, s->next_proto_negotiated, proto_len)
-            || !PACKET_get_1(&pkt, &padding_len)
-            || PACKET_remaining(&pkt) != padding_len) {
-        OPENSSL_free(s->next_proto_negotiated);
-        s->next_proto_negotiated = NULL;
-        SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, SSL_R_LENGTH_MISMATCH);
-        goto err;
-    }
+    s->next_proto_negotiated_len = (unsigned char)next_proto_len;
 
     return 1;
 err:

test/packettest.c

@@ -230,6 +230,57 @@ static int test_PACKET_copy_bytes(PACKET *pkt, size_t start)
     return 1;
 }
 
+static int test_PACKET_memdup(PACKET *pkt, size_t start)
+{
+    unsigned char *data = NULL;
+    size_t len;
+    if (       !PACKET_goto_bookmark(pkt, start)
+            || !PACKET_memdup(pkt, &data, &len)
+            ||  len != BUF_LEN
+            ||  memcmp(data, PACKET_data(pkt), len)
+            || !PACKET_forward(pkt, 10)
+            || !PACKET_memdup(pkt, &data, &len)
+            ||  len != BUF_LEN - 10
+            ||  memcmp(data, PACKET_data(pkt), len)
+            || !PACKET_back(pkt, 1)
+            || !PACKET_memdup(pkt, &data, &len)
+            ||  len != BUF_LEN - 9
+               ||  memcmp(data, PACKET_data(pkt), len)) {
+        fprintf(stderr, "test_PACKET_memdup() failed\n");
+        OPENSSL_free(data);
+        return 0;
+    }
+
+    OPENSSL_free(data);
+    return 1;
+}
+
+static int test_PACKET_strndup()
+{
+    char buf[10], buf2[10];
+    memset(buf, 'x', 10);
+    memset(buf2, 'y', 10);
+    buf2[5] = '\0';
+    char *data = NULL;
+    PACKET pkt;
+
+    if (       !PACKET_buf_init(&pkt, (unsigned char*)buf, 10)
+            || !PACKET_strndup(&pkt, &data)
+            ||  strlen(data) != 10
+            ||  strncmp(data, buf, 10)
+            || !PACKET_buf_init(&pkt, (unsigned char*)buf2, 10)
+            || !PACKET_strndup(&pkt, &data)
+            ||  strlen(data) != 5
+            ||  strcmp(data, buf2)) {
+        fprintf(stderr, "test_PACKET_strndup failed\n");
+        OPENSSL_free(data);
+        return 0;
+    }
+
+    OPENSSL_free(data);
+    return 1;
+}
+
 static int test_PACKET_move_funcs(PACKET *pkt, size_t start)
 {
     unsigned char *byte;
@@ -388,6 +439,8 @@ int main(int argc, char **argv)
             || !test_PACKET_get_sub_packet(&pkt, start)
             || !test_PACKET_get_bytes(&pkt, start)
             || !test_PACKET_copy_bytes(&pkt, start)
+            || !test_PACKET_memdup(&pkt, start)
+            || !test_PACKET_strndup()
             || !test_PACKET_move_funcs(&pkt, start)
             || !test_PACKET_get_length_prefixed_1()
             || !test_PACKET_get_length_prefixed_2()