CVE-2018-1160: libatalk/dsi: add correct bound checking to dsi_opense…

…ssion

The memcpy

  memcpy(&dsi->attn_quantum, dsi->commands + i + 1, dsi->commands[i]);

trusted dsi->commands[i] to specify a size that fits into dsi->attn_quantum. The
sizeof attn_quantum is four bytes. A malicious client can send a dsi->command[i]
larger than 4 bytes to begin overwriting variables in the DSI struct.

dsi->command[i] is a single char in a char array which limits the amount of data
the attacker can overwrite in the DSI struct to 0xff. So for this to be useful
in an attack there needs to be something within the 0xff bytes that follow
attn_quantum. From dsi.h:

    uint32_t attn_quantum, datasize, server_quantum;
    uint16_t serverID, clientID;
    uint8_t  *commands; /* DSI recieve buffer */
    uint8_t  data[DSI_DATASIZ];    /* DSI reply buffer */

The commands pointer is a heap allocated pointer that is reused for every packet
received and sent. Using the memcpy, an attacker can overwrite this to point to
an address of their choice and then all subsequent AFP packets will be written
to that location.

If the attacker chose the preauth_switch buffer, overwriting the function
pointer there with functions pointers of his choice, he can invoke this
functions over the network,

Signed-off-by: Ralph Boehme <slow@samba.org>
Reviewed-by: HAT <hat@fa2.so-net.ne.jp>
Reviewed-by: Andrew Stormont <andyjstormont@gmail.com>
(cherry picked from commit b6895be)

libatalk/dsi/dsi_opensess.c

@@ -23,24 +23,41 @@ void dsi_opensession(DSI *dsi)
   uint32_t servquant;
   uint32_t replcsize;
   int offs;
+  uint8_t cmd;
+  size_t option_len;
 
   if (setnonblock(dsi->socket, 1) < 0) {
       LOG(log_error, logtype_dsi, "dsi_opensession: setnonblock: %s", strerror(errno));
       AFP_PANIC("setnonblock error");
   }
 
   /* parse options */
-  while (i < dsi->cmdlen) {
-    switch (dsi->commands[i++]) {
+  while (i + 1 < dsi->cmdlen) {
+    cmd = dsi->commands[i++];
+    option_len = dsi->commands[i++];
+
+    if (i + option_len > dsi->cmdlen) {
+      LOG(log_error, logtype_dsi, "option %"PRIu8" too large: %zu",
+          cmd, option_len);
+      exit(EXITERR_CLNT);
+    }
+
+    switch (cmd) {
     case DSIOPT_ATTNQUANT:
-      memcpy(&dsi->attn_quantum, dsi->commands + i + 1, dsi->commands[i]);
+      if (option_len != sizeof(dsi->attn_quantum)) {
+        LOG(log_error, logtype_dsi, "option %"PRIu8" bad length: %zu",
+            cmd, option_len);
+        exit(EXITERR_CLNT);
+      }
+      memcpy(&dsi->attn_quantum, &dsi->commands[i], option_len);
       dsi->attn_quantum = ntohl(dsi->attn_quantum);
 
     case DSIOPT_SERVQUANT: /* just ignore these */
     default:
-      i += dsi->commands[i] + 1; /* forward past length tag + length */
       break;
     }
+
+    i += option_len;
   }
 
   /* let the client know the server quantum. we don't use the