Add modified certificate DNS check #1

KarthikaRajendran11 · 2022-03-14T13:13:54Z

No description provided.

rubens21 · 2022-03-14T13:38:16Z

tlscommon.c

@@ -626,13 +626,38 @@ static int certattr_matchip(GENERAL_NAME *gn, struct certattrmatch *match){
        && !memcmp(ASN1_STRING_get0_data(gn->d.iPAddress), &match->ipaddr, l)) ? 1 : 0 ;
 }

+static int compareWithModifiedHostname(char* certDNS, char* scriptHostName) {
+    int len = 0;
+    int len1 = 0, len2 = 0;


@KarthikaRajendran11 , these variables' names could be more meaningful. The current ones make it hard to follow the logic.

DaveTuchlinsky

Has this been tested with multiple DNS names in the certificate? For example, SingleDigits contains 3af521.net, *.3af521.net, 3af521.com, *.3af521.com in the SAN.

tlscommon.c

KarthikaRajendran11 · 2022-03-15T18:31:34Z

Has this been tested with multiple DNS names in the certificate? For example, SingleDigits contains 3af521.net, *.3af521.net, 3af521.com, *.3af521.com in the SAN.

If the host name returned by the script is idp.3af521.net, then it will match only to *.3af521.net and idp.3af521.net
It wont match for idp.3af521.com or *.3af521.com

dave-greenfieldlabs · 2022-03-16T13:42:06Z

tlscommon.c

+    char* pos;
+    char* modifiedScriptHostName;
+    char *result;
+    char generic[] = "*";


Add null checks for the certDNS, and scriptHostName pointers. Initialize each variable (pos, modifiedScriptHostName, and result) to a known value, specifically NULL.

dave-greenfieldlabs · 2022-03-16T13:47:55Z

tlscommon.c

+        return 0;
+    }
+    len = strlen(pos);
+    modifiedScriptHostName = malloc(len + 1);


(1) Use calloc() instead (it creates a zero'd memory buffer instead of random bytes that malloc gives you).
(2) Check for memory allocation failures. Specifically malloc/calloc return NULL when the system is out of memory, check for this, and bail.
(3) You can eliminate L646 as well, since the array will be all zeros.

dave-greenfieldlabs · 2022-03-16T13:49:30Z

tlscommon.c

+    len = strlen(pos);
+    modifiedScriptHostName = malloc(len + 1);
+    memcpy(modifiedScriptHostName, pos, len);
+    modifiedScriptHostName[len] = '\0';


Remove (per earlier comment)

dave-greenfieldlabs · 2022-03-16T13:50:56Z

tlscommon.c

+    modifiedScriptHostName = malloc(len + 1);
+    memcpy(modifiedScriptHostName, pos, len);
+    modifiedScriptHostName[len] = '\0';
+    debug(DBG_DBG, "modifiedScriptHostName : %s", modifiedScriptHostName);


modifiedScriptHostName is an attacker (think "specially crafted malicious certificate") controlled. Let's not print out the certificate info, unless we REALLY need to.

dave-greenfieldlabs · 2022-03-16T13:55:47Z

tlscommon.c

+    debug(DBG_DBG, "modifiedScriptHostName : %s", modifiedScriptHostName);
+    lenOfGeneric = strlen(generic);
+    lenOfModifiedScriptHostName = strlen(modifiedScriptHostName);
+    result = malloc(lenOfGeneric + lenOfModifiedScriptHostName + 1); 


Again, use calloc() here, and check for out of memory conditions.

dave-greenfieldlabs · 2022-03-16T14:14:02Z

tlscommon.c

+    result = malloc(lenOfGeneric + lenOfModifiedScriptHostName + 1); 
+    memcpy(result, generic, lenOfGeneric);
+    memcpy(result + lenOfGeneric, modifiedScriptHostName, lenOfModifiedScriptHostName + 1);
+    if (strlen(certDNS) == strlen(result) && memcmp(result, certDNS, strlen(certDNS)) == 0) {


My preferred way to code this would be. It's less likely to leak memory as changes are made to the code base over time.

if (strlen(...) == strlen(...)....) {
ret = 1
} else {
ret = 0
}

if (result) {
free(result);
result = NULL;
}

if (modifiedScriptHostName) {
free(modifiedScriptHostName);
modifiedScriptHostName = NULL;
}

return ret;

dave-greenfieldlabs · 2022-03-16T14:17:39Z

tlscommon.c

+    lenOfModifiedScriptHostName = strlen(modifiedScriptHostName);
+    result = malloc(lenOfGeneric + lenOfModifiedScriptHostName + 1); 
+    memcpy(result, generic, lenOfGeneric);
+    memcpy(result + lenOfGeneric, modifiedScriptHostName, lenOfModifiedScriptHostName + 1);


Remove the +1 at the end, since calloc will give you a zero'd array. If for some reason modifiedScriptHostName isn't null terminated, then you're not corrupting the result buffer along the way. This guarantees that result will be null terminated.

dave-greenfieldlabs

Have you considered the following certificate/DNS testing issues?
Please read https://datatracker.ietf.org/doc/html/rfc6125#section-6.4

DNS comparisons are case insensitive ASCII, the code looks like idp.EXAMPLE.COM won't match to *.example.com and it should.
Wildcard certificates cannot match on strings like "bar.*.example.net". Only the first level should be compared.
Internationalized Domain Names have special rules, and pre-processing requirements.

tlscommon.c

DaveTuchlinsky · 2022-03-21T13:04:53Z

tests/t_verify_cert.c

@@ -568,7 +543,6 @@ vY/uPjA=\n\
    X509_free(certsanipv6);
    X509_free(certsanipv6indns);
    X509_free(certcomplex);
-    X509_free(certcomplexother);


Why was this certificate removed?

It is not being used in any tests. They are just allocating it and freeing

DaveTuchlinsky · 2022-03-21T13:07:58Z

tlscommon.c

 static int _general_name_regex_match(char *v, int l, struct certattrmatch *match) {
    char *s;
    if (l <= 0 ) 
        return 0;
    if (match->exact) {
        if (l == strlen(match->exact) && memcmp(v, match->exact, l) == 0)
            return 1;
+        if(compareWithModifiedHostname(v, match->exact)) {


Why is this in the match->exact block? If it must be here, then please include a comment on why since it mutates the expected behaviour of an exact match.

This is the original code format. If match->exact is set it means we have found a host/DNS (the script has returned a valid host).

DaveTuchlinsky · 2022-03-21T13:11:22Z

tlscommon.c

@@ -626,13 +628,182 @@ static int certattr_matchip(GENERAL_NAME *gn, struct certattrmatch *match){
        && !memcmp(ASN1_STRING_get0_data(gn->d.iPAddress), &match->ipaddr, l)) ? 1 : 0 ;
 }

+static char* copy(char* s1, char* s2) {


This func is not copy. It is allocating & concatenating.

tlscommon.c

dave-greenfieldlabs · 2022-03-21T14:56:31Z

tlscommon.c

+    if(s1 == NULL && s2 == NULL) {
+        return NULL;
+    }
+    char* result = (char*) calloc(strlen(s1) + strlen(s2), sizeof(char)); 


Off by one error, should be strlen(s1)+strlen(s2)+1.

dave-greenfieldlabs · 2022-03-21T14:56:48Z

tlscommon.c

+    if(s2 == NULL) {
+        return s1;
+    }
+    if(s1 == NULL && s2 == NULL) {


Should be the first boolean test made.

dave-greenfieldlabs · 2022-03-21T15:00:20Z

tlscommon.c

@@ -626,13 +628,185 @@ static int certattr_matchip(GENERAL_NAME *gn, struct certattrmatch *match){
        && !memcmp(ASN1_STRING_get0_data(gn->d.iPAddress), &match->ipaddr, l)) ? 1 : 0 ;
 }

+static char* copy(char* s1, char* s2) {
+    if(s1 == NULL) {
+        return s2;


Dangerous, this breaks the schematics of the function. Returning s2 doesn't return a deep copy (or clone) of s2, but a pointer to s2. Doing do would cause the memory to be improperly deallocated as shown below...

char* mynewstring = copy("Foo", "Bar"); // use mynewstring for a while, then deallocate it. free(mynewstring); mynewstring=NULL; char* thisisbad = copy(NULL, "Foo"); // use thisisbad free(thisisbad); // Attempt to free the pointer to the string "Foo" which ISN'T heap allocated.

You should allocate new memory for "Foo" from the heap (with calloc) and then copy it over.

dave-greenfieldlabs · 2022-03-21T15:03:40Z

tlscommon.c

+   char* result = NULL;
+   for(i = 0; i < n; i++) {
+      val = va_arg(ap, char*);
+      result = copy(result, val);


Where is result free'd? What happens if it's NULL?

append is called from compareWithModifiedHostname and calculateLenOfLabel (which is called from compareWithModifiedHostname). NULL check is done in compareWithModifiedHostname. And the memory used is also freed in compareWithModifiedHostname. modifiedHostName, pattern, lenRegex are the variables that are results of append. They are freed in compareWithModifiedHostname.

dave-greenfieldlabs · 2022-03-21T15:05:21Z

tlscommon.c

+
+    int reti = regcomp(&regex, pattern, REG_EXTENDED);
+    if (reti) {
+        regerror(reti, &regex, buffer, 100);


instead of using 100 the constant, should be sizeof(buffer) instead

dave-greenfieldlabs · 2022-03-21T15:14:47Z

tlscommon.c

+        return result;
+    }
+    int lenOfFirstPortion = strlen(dns) - strlen(firstPos);
+    result = (char*) calloc(lenOfFirstPortion, sizeof(char));


Off by 1, lenOfFirstPortion + 1

dave-greenfieldlabs · 2022-03-21T16:02:18Z

tlscommon.c

+   char* result = NULL;
+   for(i = 0; i < n; i++) {
+      val = va_arg(ap, char*);
+      result = concatenate(result, val);


Where does result get free()'d

dave-greenfieldlabs · 2022-03-21T16:06:23Z

tlscommon.c

+
+static char* calculateLenOfLabel(int len) {
+    char str[3];
+    sprintf(str, "%d", len);


Buffer overflow here. len can (in general) be the size of an integer. So you need more than 3 bytes to store a 32-bit decimal value.

dave-greenfieldlabs · 2022-03-21T16:08:28Z

tlscommon.c

+    char str[3];
+    sprintf(str, "%d", len);
+    debug(DBG_DBG, "str : %s", str);
+    char* result = append(3, "{0,", str, "}");


Is 3 really what we want?

dave-greenfieldlabs · 2022-03-21T16:17:45Z

tlscommon.c

+// Internationlized Domain Names: www.äöü.com converted to www.xn--4ca0bs.com. xn--4ca0bs is considered
+// a valid DN and let through. diseñolatinoamericano.com => xn--diseolatinoamericano-66b.com will also be
+// let through. So IDNs are passed in Punycode encoded and xn-- says everything that follows is unicode encoded.
+static int compareWithModifiedHostname(char* certDNS, char* hostname) {


Karthika, there is a much safer memory pattern to use here. The reason for the pattern below is that it keeps all memory deallocations in one place for better readability, and better maintainability. This is 'C' version of exception handling (sort of). Consider the following pattern...

char* firstPorCertDNS = NULL; char* firstPorHostName = NULL; char* modifiedHostName = NULL; int retcode = ERROR; .... do stuff .... if (I detected and error) { goto cleanup; } ... do more stuff ... retcode = 1; return retcode; cleanup: if (firstPorCertDNS != NULL) { free(firstPorCertDNS); firstPorCertDNS = NULL; } if (firstPorHostName != NULL) { free(firstPorHostName); firstPorHostName = NULL; } if (modifiedHostName != NULL) { free(modifiedHostName); modifiedHostName = NULL; } return retcode; }

DaveTuchlinsky · 2022-03-31T13:45:07Z

tlscommon.c

+        retCode = 1;
+    }
+
+    goto cleanup;


Unneeded & spacing below is off. Labels are outdented.

KarthikaRajendran11 added 2 commits March 12, 2022 14:21

Add modified certificate DNS check

72b95c0

Remove log

06d5c57

rubens21 reviewed Mar 14, 2022

View reviewed changes

KarthikaRajendran11 added 2 commits March 14, 2022 10:51

Address PR comments

1b6cc4f

Add comment

0bb9d01

DaveTuchlinsky reviewed Mar 15, 2022

View reviewed changes

tlscommon.c Outdated Show resolved Hide resolved

tlscommon.c Outdated Show resolved Hide resolved

tlscommon.c Outdated Show resolved Hide resolved

KarthikaRajendran11 added 2 commits March 15, 2022 14:47

Address PR comments

e37c4f7

Check for IP v4 or v6 inputs

e84afdf

KarthikaRajendran11 requested a review from dave-greenfieldlabs March 15, 2022 19:06

dave-greenfieldlabs requested changes Mar 16, 2022

View reviewed changes

KarthikaRajendran11 added 7 commits March 16, 2022 16:24

Address PR comments

34cea36

Adding regex matching

ddd2f57

Add libraries

2706fd0

Fix tests

6f6b68c

Cleanup

ce1ddfb

Cleanup

1b0e791

Add IDN examples in code

92b5a34

DaveTuchlinsky requested changes Mar 21, 2022

View reviewed changes

Free regex1; Rename fn

521d294

dave-greenfieldlabs requested changes Mar 21, 2022

View reviewed changes

KarthikaRajendran11 added 4 commits March 23, 2022 10:07

Use goto blocks

5172582

Modify calculateLenOfLabel

1511ae1

Cleanup

834d6c9

Replace 1st memcpy with strcpy

4f8c53e

DaveTuchlinsky reviewed Mar 31, 2022

View reviewed changes

tlscommon.c

retCode = 1;

}

goto cleanup;

Copy link

DaveTuchlinsky Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded & spacing below is off. Labels are outdented.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modified certificate DNS check #1

Add modified certificate DNS check #1

KarthikaRajendran11 commented Mar 14, 2022

rubens21 Mar 14, 2022

DaveTuchlinsky left a comment

KarthikaRajendran11 commented Mar 15, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs Mar 16, 2022

dave-greenfieldlabs left a comment

DaveTuchlinsky Mar 21, 2022

KarthikaRajendran11 Mar 21, 2022

DaveTuchlinsky Mar 21, 2022

KarthikaRajendran11 Mar 21, 2022

DaveTuchlinsky Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

KarthikaRajendran11 Mar 22, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

dave-greenfieldlabs Mar 21, 2022

DaveTuchlinsky Mar 31, 2022

Add modified certificate DNS check #1

Are you sure you want to change the base?

Add modified certificate DNS check #1

Conversation

KarthikaRajendran11 commented Mar 14, 2022

Choose a reason for hiding this comment

DaveTuchlinsky left a comment

Choose a reason for hiding this comment

KarthikaRajendran11 commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dave-greenfieldlabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment