Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix [bad6cc213d]: A format string vulnerability in Tcl nmakehelp.c al…
…lows code execution via a crated file.

Also change a memcpy() to a memmove(), because the range could be overlapping
  • Loading branch information
jan.nijtmans committed Jun 22, 2021
1 parent d445789 commit 4705dbd
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions win/nmakehlp.c
Expand Up @@ -537,7 +537,7 @@ GetVersionFromFile(
++q;
}

memcpy(szBuffer, p, q - p);
memmove(szBuffer, p, q - p);
szBuffer[q-p] = 0;
szResult = szBuffer;
break;
Expand Down Expand Up @@ -674,7 +674,7 @@ SubstituteFile(
memcpy(szBuffer, szCopy, sizeof(szCopy));
}
}
printf(szBuffer);
printf("%s", szBuffer);
}

list_free(&substPtr);
Expand Down

7 comments on commit 4705dbd

@salmonx
Copy link

@salmonx salmonx commented on 4705dbd Jul 1, 2021

Choose a reason for hiding this comment

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

CVE-2021-35331 was assigned for this issue.

@sebres
Copy link
Member

@sebres sebres commented on 4705dbd Jul 1, 2021

Choose a reason for hiding this comment

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

jeez! do you really think that a bug in a build helper actually need a CVE entry?

@nijtmans
Copy link
Member

Choose a reason for hiding this comment

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

The CVE has the status DISPUTED now. I share DHR's opinion that - even though it's indeed a bug - it cannot be used for any "code execution", since this file is only a build helper. Fixing it is no work at all (proof: this commit), any discussion about it has no further value for me.

@sebres
Copy link
Member

@sebres sebres commented on 4705dbd Jul 6, 2021

Choose a reason for hiding this comment

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

A discussion (if argumentative) has always a value, Jan...
Just I don't think that someone would be able to illustrate any even a theoretical attack vector by its usage at the moment.

@ilatypov
Copy link

@ilatypov ilatypov commented on 4705dbd Nov 25, 2022

Choose a reason for hiding this comment

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

The argument "only a build helper" does not directly address the danger of code execution. I came here from a CVE alert and I believe the likeliest most impactful attack could involve a build farm machine. The nmakehlp was intended as a Windows tool but I guess for the sake of reusability, the same tool may be used on a Linux CI/CD farm machine.

  • CONDITION: nmakehlp is vulnerable.
    • CONDITION: A build system runs nmakehlp to replace token patterns with secrets from a "substitution file" when building, testing or deploying a project, nmakehlp -s SECRETS_FILE CONFIG_TEMPLATE_FILE > CONFIG_FILE
      • CONDITION: The build system OR the source code repository allows a relatively minor project role member to add arbitrary data to the contents of SECRETS_FILE or CONFIG_TEMPLATE_FILE.
        • CONDITION: Intruders obtain (opportunistically or intentionally) a temporary or permanent identity of a member of the minor role.
          • IMPACT: intruders can execute arbitrary code on the build farm's machine.

Depending on whether the conditions are possible, the risk of the vulnerability before its fix may vary from zero to critical.

The existing code has another visible vulnerability where the szCopy's running pointer is written to without checking for the buffer overrun on lines 659 and 661,

tcl/win/nmakehlp.c

Lines 645 to 667 in 2192064

/*
* Run the substitutions over each line of the input
*/
while (fgets(szBuffer, sizeof(szBuffer), fp) != NULL) {
list_item_t *p = NULL;
for (p = substPtr; p != NULL; p = p->nextPtr) {
char *m = strstr(szBuffer, p->key);
if (m) {
char *cp, *op, *sp;
cp = szCopy;
op = szBuffer;
while (op != m) *cp++ = *op++;
sp = p->value;
while (sp && *sp) *cp++ = *sp++;
op += strlen(p->key);
while (*op) *cp++ = *op++;
*cp = 0;
memcpy(szBuffer, szCopy, sizeof(szCopy));
}
}
printf("%s", szBuffer);
}

@sebres
Copy link
Member

@sebres sebres commented on 4705dbd Nov 25, 2022

Choose a reason for hiding this comment

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

  1. If one's able to build (use a compile/make toolchain) one can do almost everything (create own executable you want), so it doesn't really matter in that case;
    The same is valid if the source code is under your full control (someone can simply change it to make it vulnerable by the next build process).
  2. If it is already built (the executable is there), the same impact is imaginable if usage of any tool is basically allowed (think about rm -rf ... and co);
  3. If someone uses this helper (or even another tool) in a way that a foreign input can be supplied as a parameter and it is not pre-validated (like "allows a relatively minor project role member to add arbitrary data"),
    it is the matter of (invalid) usage and one cannot blame the tool for that;
    Likewise if someone allows to add arbitrary data (without to validate it) as a parameter to the build-scripts, no matter where, it is pretty simple to achieve similar impact by many stable tools (without known "vulnerabilities").
    For instance because one could force it to do something unexpected (theoretically even to build some other tool, see P. 1, and then use it hereafter).

Anyway in provided scenario the issue will be rather the usage of unfiltered foreign input for a parameter supplied to nmakehlp. Not the "vulnerability" of the nmakehlp self.
Especially on windows, which has notoriously a vulnerable command processor, so you don't really want to invoke something on windows without parameter validation.

But again, I am not against the "fix" at all, just to explain why calling that a vulnerability and provide a CVE is a bit too heavy for that kind of "tool", well basically build-helper.
However you can provide your fix (patch) to core.tcl-lang.org.

@ilatypov
Copy link

@ilatypov ilatypov commented on 4705dbd Nov 25, 2022

Choose a reason for hiding this comment

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

I remember that POODLE was also limited to attackers having a peculiar relationship with the data they were attacking. Thanks to the intricacy of the web usage patterns established in the past decades, the relationship did make sense. The attacker would need to
(A) have partial write-only access to the plaintext data. This was made possible by getting the victims open a malicious site whose UI would start sending requests to the application server. The attacker would also need to
(B) have read/write access to the encrypted data in transit (possibly by setting up a fake WiFi hotspot).

Despite so many IFs, the vulnerability was considered serious. I agree that nmakehlp's scope of use is billion times smaller than that of TLS. Thanks for providing additional points against the likelihood of the attack (I may disagree about technicalities. The exploit does not require access to command line modifications. It only needs partial write-only access to one of the 2 files via either the source code repository or a build machine's other data flow). I agree often times having partial write access comes with many other permissions and attacks that would make the abuse of nmakehlp unfeasible. These pro et contra arguments help in evaluating the cost/benefit of upgrading/keeping the Tcl version.

Please sign in to comment.