Don't use bio_writer_t.skip() to write length field when appending more data

If the writer reallocates its buffer, the length pointer might not be valid
anymore, or even worse, point to an arbitrary allocation.

diff --git a/src/libcharon/encoding/payloads/eap_payload.c b/src/libcharon/encoding/payloads/eap_payload.c
index dd2e257..f2f35aa 100644 (file)
--- a/src/libcharon/encoding/payloads/eap_payload.c
+++ b/src/libcharon/encoding/payloads/eap_payload.c
@@ -410,14 +410,15 @@ eap_payload_t *eap_payload_create_nak(u_int8_t identifier, eap_type_t type,
        eap_type_t reg_type;
        u_int32_t reg_vendor;
        bio_writer_t *writer;
-       chunk_t length, data;
+       chunk_t data;
        bool added_any = FALSE, found_vendor = FALSE;
        eap_payload_t *payload;
 
        writer = bio_writer_create(12);
        writer->write_uint8(writer, EAP_RESPONSE);
        writer->write_uint8(writer, identifier);
-       length = writer->skip(writer, 2);
+       /* write zero length, we update it once we know the length */
+       writer->write_uint16(writer, 0);
 
        write_type(writer, EAP_NAK, 0, expanded);
 
@@ -453,10 +454,9 @@ eap_payload_t *eap_payload_create_nak(u_int8_t identifier, eap_type_t type,
 
        /* set length */
        data = writer->get_buf(writer);
-       htoun16(length.ptr, data.len);
+       htoun16(data.ptr + offsetof(eap_packet_t, length), data.len);
 
        payload = eap_payload_create_data(data);
        writer->destroy(writer);
        return payload;
 }
-

diff --git a/src/libstrongswan/bio/bio_writer.h b/src/libstrongswan/bio/bio_writer.h
index 57a5c3d..2ac4f35 100644 (file)
--- a/src/libstrongswan/bio/bio_writer.h
+++ b/src/libstrongswan/bio/bio_writer.h
@@ -126,8 +126,11 @@ struct bio_writer_t {
        void (*wrap32)(bio_writer_t *this);
 
        /**
-        * Skips len bytes in the buffer before the next data is written, returns
-        * a chunk covering the skipped bytes.
+        * Skips len bytes in the buffer, return chunk of skipped data.
+        *
+        * The returned chunk is not valid after calling any other writer function
+        * (except get_buf()), because a buffer reallocation might move the
+        * internal buffer to a different memory location!
         *
         * @param len           number of bytes to skip
         * @return                      chunk pointing to skipped bytes in the internal buffer