[exim-cvs] Fix SPA authenticator. Bug 3106

Góra strony
Delete this message
Reply to this message
Autor: Exim Git Commits Mailing List
Data:  
Dla: exim-cvs
Temat: [exim-cvs] Fix SPA authenticator. Bug 3106
Gitweb: https://git.exim.org/exim.git/commitdiff/a731c6050a1510734776851aaff5ad2f32fa3ae5
Commit:     a731c6050a1510734776851aaff5ad2f32fa3ae5
Parent:     19c4ea037946e197e30530bd6b4f2880be77c95f
Author:     Jeremy Harris <jgh146exb@???>
AuthorDate: Mon Aug 5 12:51:12 2024 +0100
Committer:  Jeremy Harris <jgh146exb@???>
CommitDate: Mon Aug 5 19:41:11 2024 +0100


    Fix SPA authenticator.  Bug 3106
---
 doc/doc-txt/ChangeLog    |   6 ++
 src/src/auths/auth-spa.c | 205 +++++++++++++++++++----------------------------
 src/src/auths/auth-spa.h |  17 ++--
 src/src/auths/spa.c      |   2 +-
 4 files changed, 99 insertions(+), 131 deletions(-)


diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index a02911cae..7c888dbc0 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -31,6 +31,12 @@ JH/06 Bug 1141: When operating a continued-connection transport, verify that
       the interface option, if specified, evaluates to match the connection.
       Previously, a queued message for the same host was sent without checking.


+JH/07 Bug 3106: Fix coding in SPA authenticator. A macro argument was not
+      properly parenthesized, resulting in a logic error.  While the simple
+      fix was provided by Andrew Aitchison, the over-large code block resulting
+      from this macro made me want to replace it with a real function so more
+      extensive rework becamse needed.
+
 Exim version 4.98
 -----------------


diff --git a/src/src/auths/auth-spa.c b/src/src/auths/auth-spa.c
index fd3099034..96e7148ab 100644
--- a/src/src/auths/auth-spa.c
+++ b/src/src/auths/auth-spa.c
@@ -1203,55 +1203,67 @@ char versionString[] = "libntlm version 0.21";
/* Utility routines that handle NTLM auth structures. */

/* The [IS]VAL macros are to take care of byte order for non-Intel
- * Machines -- I think this file is OK, but it hasn't been tested.
- * The other files (the ones stolen from Samba) should be OK.
- */
+Machines -- I think this file is OK, but it hasn't been tested.
+The other files (the ones stolen from Samba) should be OK. */


-/* I am not crazy about these macros -- they seem to have gotten
- * a bit complex. A new scheme for handling string/buffer fields
- * in the structures probably needs to be designed
- */
+/* Append a string to the buffer and point the header struct at that. */

-#define spa_bytes_add(ptr, header, buf, count) \
-{ \
-if (  buf && (count) != 0    /* we hate -Wint-in-bool-contex */ \
-   && ptr->bufIndex + count < sizeof(ptr->buffer)        \
-   ) \
-  { \
-  SSVAL(&ptr->header.len,0,count); \
-  SSVAL(&ptr->header.maxlen,0,count); \
-  SIVAL(&ptr->header.offset,0,((ptr->buffer - ((uint8x*)ptr)) + ptr->bufIndex)); \
-  memcpy(ptr->buffer+ptr->bufIndex, buf, count); \
-  ptr->bufIndex += count; \
-  } \
-else \
-  { \
-  ptr->header.len = \
-  ptr->header.maxlen = 0; \
-  SIVAL(&ptr->header.offset,0,((ptr->buffer - ((uint8x*)ptr)) + ptr->bufIndex)); \
-  } \
+static void
+spa_bytes_add(SPAbuf * buffer, size_t off, SPAStrHeader * header,
+  const uschar * src, int count)
+{
+off += buffer->bufIndex;
+if (  src && count != 0            /* we hate -Wint-in-bool-contex */
+   && buffer->bufIndex + count < sizeof(buffer->buffer)    
+   )
+  {
+  SSVAL(&header->len, 0, count);
+  SSVAL(&header->maxlen, 0, count);
+  SIVAL(&header->offset, 0, off);
+  memcpy(buffer->buffer + buffer->bufIndex, src, count);
+  buffer->bufIndex += count;
+  }
+else
+  {
+  header->len = header->maxlen = 0;
+  SIVAL(&header->offset, 0, off);
+  }
 }


-#define spa_string_add(ptr, header, string) \
-{ \
-uschar * p = string; \
-int len = 0; \
-if (p) len = Ustrlen(p); \
-spa_bytes_add(ptr, header, p, len); \
+static void
+spa_string_add(SPAbuf * buffer, size_t off, SPAStrHeader * header,
+ const uschar * string)
+{
+int len = string ? Ustrlen(string) : 0;
+spa_bytes_add(buffer, off, header, string, len);
+}
+
+static uschar *
+strToUnicode(const uschar * p)
+{
+static uschar buf[1024];
+size_t l = Ustrlen(p);
+
+assert (l * 2 < sizeof buf);
+
+for (int i = 0; l--; ) { buf[i++] = *p++; buf[i++] = 0; }
+return buf;
}

-#define spa_unicode_add_string(ptr, header, string) \
-{ \
-uschar * p = string; \
-uschar * b = NULL; \
-int len = 0; \
-if (p) \
- { \
- len = Ustrlen(p); \
- b = US strToUnicode(CS p); \
- } \
-spa_bytes_add(ptr, header, b, len*2); \
+static void
+spa_unicode_add_string(SPAbuf * buffer, size_t off, SPAStrHeader * header,
+ const uschar * string)
+{
+const uschar * p = string;
+uschar * b = NULL;
+int len = 0;
+if (p)
+ {
+ len = Ustrlen(p);
+ b = US strToUnicode(p);
+ }
+spa_bytes_add(buffer, off, header, b, len*2);
}


@@ -1292,24 +1304,6 @@ buf[i] = '\0';
return buf;
}

-static uschar *
-strToUnicode (char *p)
-{
-static uschar buf[1024];
-size_t l = strlen (p);
-int i = 0;
-
-assert (l * 2 < sizeof buf);
-
-while (l--)
- {
- buf[i++] = *p++;
- buf[i++] = 0;
- }
-
-return buf;
-}
-
static uschar *
toString (char *p, size_t len)
{
@@ -1404,12 +1398,14 @@ if (p)
*p = '\0';
}

-request->bufIndex = 0;
+request->buf.bufIndex = 0;
 memcpy (request->ident, "NTLMSSP\0\0\0", 8);
 SIVAL (&request->msgType, 0, 1);
 SIVAL (&request->flags, 0, 0x0000b207);      /* have to figure out what these mean */
-spa_string_add (request, user, u);
-spa_string_add (request, domain, domain);
+spa_string_add(&request->buf, offsetof(SPAAuthRequest, buf), &request->user,
+        u);
+spa_string_add(&request->buf, offsetof(SPAAuthRequest, buf), &request->domain,
+        domain);
 }



@@ -1427,7 +1423,7 @@ patch added by PH on suggestion of Russell King */

memset(challenge, 0, sizeof(SPAAuthChallenge));

-challenge->bufIndex = 0;
+challenge->buf.bufIndex = 0;
memcpy (challenge->ident, "NTLMSSP\0", 8);
SIVAL (&challenge->msgType, 0, 2);
SIVAL (&challenge->flags, 0, 0x00008201);
@@ -1449,54 +1445,12 @@ memcpy(challenge->challengeData,chalstr,8);



-/* This is the original source of this function, preserved here for reference.
+/* The original version of this function is available in git.
 The new version below was re-organized by PH following a patch and some further
 suggestions from Mark Lyda to fix the problem that is described at the head of
 this module. At the same time, I removed the untidiness in the code below that
-involves the "d" and "domain" variables. */
-
-#ifdef NEVER
-void
-spa_build_auth_response (SPAAuthChallenge * challenge,
-                        SPAAuthResponse * response, char *user,
-                        char *password)
-{
-uint8x lmRespData[24];
-uint8x ntRespData[24];
-char *d = strdup (GetUnicodeString (challenge, uDomain));
-char *domain = d;
-char *u = strdup (user);
-char *p = strchr (u, '@');
-
-if (p)
-  {
-  domain = p + 1;
-  *p = '\0';
-  }
-
-spa_smb_encrypt (US password, challenge->challengeData, lmRespData);
-spa_smb_nt_encrypt (US password, challenge->challengeData, ntRespData);
-
-response->bufIndex = 0;
-memcpy (response->ident, "NTLMSSP\0\0\0", 8);
-SIVAL (&response->msgType, 0, 3);
-
-spa_bytes_add (response, lmResponse, lmRespData, 24);
-spa_bytes_add (response, ntResponse, ntRespData, 24);
-spa_unicode_add_string (response, uDomain, domain);
-spa_unicode_add_string (response, uUser, u);
-spa_unicode_add_string (response, uWks, u);
-spa_string_add (response, sessionKey, NULL);
-
-response->flags = challenge->flags;
-
-free (d);
-free (u);
-}
-#endif
-
-
-/* This is the re-organized version (see comments above) */
+involves the "d" and "domain" variables.
+Further modified by JGH to replace complex macro "functions" with real ones. */


void
spa_build_auth_response (SPAAuthChallenge * challenge,
@@ -1510,6 +1464,8 @@ uschar * u = string_copy(user);
uschar * p = Ustrchr(u, '@');
uschar * d = NULL;
uschar * domain;
+SPAbuf * buf = &response->buf;
+const size_t off = offsetof(SPAAuthResponse, buf);

if (p)
{
@@ -1524,24 +1480,27 @@ else domain = d = string_copy(cf & 0x1
spa_smb_encrypt(password, challenge->challengeData, lmRespData);
spa_smb_nt_encrypt(password, challenge->challengeData, ntRespData);

-response->bufIndex = 0;
+buf->bufIndex = 0;
memcpy (response->ident, "NTLMSSP\0\0\0", 8);
SIVAL (&response->msgType, 0, 3);

-spa_bytes_add(response, lmResponse, lmRespData, cf & 0x200 ? 24 : 0);
-spa_bytes_add(response, ntResponse, ntRespData, cf & 0x8000 ? 24 : 0);
-
-if (cf & 0x1) {      /* Unicode Text */
-     spa_unicode_add_string(response, uDomain, domain);
-     spa_unicode_add_string(response, uUser, u);
-     spa_unicode_add_string(response, uWks, u);
-} else {             /* OEM Text */
-     spa_string_add(response, uDomain, domain);
-     spa_string_add(response, uUser, u);
-     spa_string_add(response, uWks, u);
-}
+spa_bytes_add(buf, off, &response->lmResponse, lmRespData, cf & 0x200 ? 24 : 0);
+spa_bytes_add(buf, off, &response->ntResponse, ntRespData, cf & 0x8000 ? 24 : 0);
+
+if (cf & 0x1)        /* Unicode Text */
+  {
+  spa_unicode_add_string(buf, off, &response->uDomain, domain);
+  spa_unicode_add_string(buf, off, &response->uUser, u);
+  spa_unicode_add_string(buf, off, &response->uWks, u);
+  }
+else
+  {            /* OEM Text */
+  spa_string_add(buf, off, &response->uDomain, domain);
+  spa_string_add(buf, off, &response->uUser, u);
+  spa_string_add(buf, off, &response->uWks, u);
+  }


-spa_string_add(response, sessionKey, NULL);
+spa_string_add(buf, off, &response->sessionKey, NULL);
response->flags = challenge->flags;
}

diff --git a/src/src/auths/auth-spa.h b/src/src/auths/auth-spa.h
index 629f50af5..b7f3c5017 100644
--- a/src/src/auths/auth-spa.h
+++ b/src/src/auths/auth-spa.h
@@ -37,6 +37,12 @@ typedef struct
        uint32x         offset;
 } SPAStrHeader;


+typedef struct
+{
+    uint8x    buffer[1024];
+    uint32x    bufIndex;
+} SPAbuf;
+
 typedef struct
 {
        char         ident[8];
@@ -46,8 +52,7 @@ typedef struct
        uint8x         challengeData[8];
        uint8x         reserved[8];
        SPAStrHeader    emptyString;
-       uint8x         buffer[1024];
-       uint32x         bufIndex;
+       SPAbuf        buf;
 } SPAAuthChallenge;



@@ -58,8 +63,7 @@ typedef struct
        uint32x         flags;
        SPAStrHeader    user;
        SPAStrHeader    domain;
-       uint8x         buffer[1024];
-       uint32x         bufIndex;
+       SPAbuf        buf;
 } SPAAuthRequest;


 typedef struct
@@ -73,11 +77,10 @@ typedef struct
        SPAStrHeader    uWks;
        SPAStrHeader    sessionKey;
        uint32x         flags;
-       uint8x         buffer[1024];
-       uint32x         bufIndex;
+       SPAbuf        buf;
 } SPAAuthResponse;


-#define spa_request_length(ptr) (((ptr)->buffer - (uint8x*)(ptr)) + (ptr)->bufIndex)
+#define spa_request_length(ptr) (((uint8x*)&(ptr)->buf - (uint8x*)(ptr)) + (ptr)->buf.bufIndex)

void spa_bits_to_base64 (unsigned char *, const unsigned char *, int);
int spa_base64_to_bits(char *, int, const char *);
diff --git a/src/src/auths/spa.c b/src/src/auths/spa.c
index 7ec3974e1..e13798f0e 100644
--- a/src/src/auths/spa.c
+++ b/src/src/auths/spa.c
@@ -195,7 +195,7 @@ that causes failure if the size of msgbuf is exceeded. ****/
int len = SVAL(&responseptr->uUser.len,0)/2;

   if (  (off = IVAL(&responseptr->uUser.offset,0)) >= sizeof(SPAAuthResponse)
-     || len >= sizeof(responseptr->buffer)/2
+     || len >= sizeof(responseptr->buf.buffer)/2
      || (p = (CS responseptr) + off) + len*2 >= CS (responseptr+1)
      )
     {


--
## subscription configuration (requires account):
## https://lists.exim.org/mailman3/postorius/lists/exim-cvs.lists.exim.org/
## unsubscribe (doesn't require an account):
## exim-cvs-unsubscribe@???
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/