Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stack-based buffer overflow findTable() (CVE-2014-8184) #425

Closed
kirotawa opened this issue Oct 16, 2017 · 14 comments
Closed

stack-based buffer overflow findTable() (CVE-2014-8184) #425

kirotawa opened this issue Oct 16, 2017 · 14 comments

Comments

@kirotawa
Copy link

Hello,

Do you have any commit for this issue?

References:
https://bugzilla.redhat.com/show_bug.cgi?id=1492701
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8184

Thanks in advance!

@kirotawa kirotawa changed the title stack-based buffer overflow (CVE-2014-8184) stack-based buffer overflow findTable() (CVE-2014-8184) Oct 16, 2017
@egli
Copy link
Member

egli commented Oct 16, 2017

No we do not have any commits regarding this issue. We know of a number of issues thanks to the Coverity scan, also there is a resource leak in lou_findTable. But I would know of a buffer overrun in lou_findTable.

@carnil
Copy link

carnil commented Nov 1, 2017

@egli, @kirotawa: Asked in https://bugzilla.redhat.com/show_bug.cgi?id=1492701#c5 if details can be shared.

@carnil
Copy link

carnil commented Nov 2, 2017

@egli, @kirotawa: according to https://bugzilla.redhat.com/show_bug.cgi?id=1492701#c7:

this vulnerability (actually several buffer overflows in that same function) was sitting in our package because it was outdated. It was probably unknowingly fixed as this function was totally refactored during this merge: dc97ef7#diff-7ade83431f79d2120c82012aee3b05c9L4524

This specific vulnerability does not exists in upstream version and it was introduced in commit 26ca861.

@kirotawa
Copy link
Author

kirotawa commented Nov 2, 2017

I tried to apply this merge patch in a 2.5.3 version (backporting) but while it builds ok it fails in the new resolve_table test. Seems that something is missing, not sure yet.

@carnil
Copy link

carnil commented Nov 2, 2017

@kirotawa: The isolated patch which Red Hat did apply can be found at
https://git.centos.org/blob/rpms!liblouis.git/9f94aa24d3308691c575e2659e42321f4aff1cf3/SOURCES!security-fixes.patch#L690
and is extracted:

From 2fe2b279994e3ed70bae461e284702cc1c7d4665 Mon Sep 17 00:00:00 2001
From: Raphael Sanchez Prudencio <rsprudencio@redhat.com>
Date: Mon, 18 Sep 2017 18:44:31 +0200
Subject: [PATCH 5/7] Fix multiple stack-based buffer overflows in findTable().

Fixes CVE-2014-8184.
---
 liblouis/compileTranslationTable.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/liblouis/compileTranslationTable.c b/liblouis/compileTranslationTable.c
index ec4963f0..25c0208f 100644
--- a/liblouis/compileTranslationTable.c
+++ b/liblouis/compileTranslationTable.c
@@ -4502,8 +4502,7 @@ findTable (const char *tableName)
   char trialPath[MAXSTRING];
   if (tableName == NULL || tableName[0] == 0)
     return NULL;
-  strcpy (trialPath, tablePath);
-  strcat (trialPath, tableName);
+  snprintf (trialPath, MAXSTRING-1, "%s%s", tablePath, tableName);
   if ((tableFile = fopen (trialPath, "rb")))
     return tableFile;
   pathEnd[0] = DIR_SEP;
@@ -4522,18 +4521,15 @@ findTable (const char *tableName)
 	    break;
 	if (k == listLength || k == 0)
 	  {			/* Only one file */
-	    strcpy (trialPath, pathList);
-	    strcat (trialPath, pathEnd);
-	    strcat (trialPath, tableName);
+	    snprintf (trialPath, MAXSTRING-1, "%s%s%s", pathList, pathEnd, tableName);
 	    if ((tableFile = fopen (trialPath, "rb")))
 	      break;
 	  }
 	else
 	  {			/* Compile a list of files */
-	    strncpy (trialPath, pathList, k);
-	    trialPath[k] = 0;
-	    strcat (trialPath, pathEnd);
-	    strcat (trialPath, tableName);
+	    char path[MAXSTRING];
+	    strncpy (path, pathList, k);
+	    snprintf (trialPath, MAXSTRING-1, "%s%s%s", path, pathEnd, tableName);
 	    currentListPos = k + 1;
 	    if ((tableFile = fopen (trialPath, "rb")))
 	      break;
@@ -4542,11 +4538,8 @@ findTable (const char *tableName)
 		for (k = currentListPos; k < listLength; k++)
 		  if (pathList[k] == ',')
 		    break;
-		strncpy (trialPath,
-			 &pathList[currentListPos], k - currentListPos);
-		trialPath[k - currentListPos] = 0;
-		strcat (trialPath, pathEnd);
-		strcat (trialPath, tableName);
+		strncpy (path, &pathList[currentListPos], k - currentListPos);
+		snprintf (trialPath, MAXSTRING-1, "%s%s%s", path, pathEnd, tableName);
 		if ((tableFile = fopen (trialPath, "rb")))
 		  currentListPos = k + 1;
 		break;
@@ -4564,26 +4557,20 @@ findTable (const char *tableName)
   pathList = lou_getDataPath ();
   if (pathList)
     {
-      strcpy (trialPath, pathList);
-      strcat (trialPath, pathEnd);
 #ifdef _WIN32
-      strcat (trialPath, "liblouis\\tables\\");
+      snprintf (trialPath, MAXSTRING-1, "%s%sliblouis\\tables\\%s", pathList, pathEnd, tableName);
 #else
-      strcat (trialPath, "liblouis/tables/");
+      snprintf (trialPath, MAXSTRING-1, "%s%sliblouis/tables/%s", pathList, pathEnd, tableName);
 #endif
-      strcat (trialPath, tableName);
       if ((tableFile = fopen (trialPath, "rb")))
 	return tableFile;
     }
   /* See if table on installed or program path. */
 #ifdef _WIN32
-  strcpy (trialPath, lou_getProgramPath ());
-  strcat (trialPath, "\\share\\liblouss\\tables\\");
+  snprintf (trialPath, MAXSTRING-1, "%s\\share\\liblouss\\tables\\%s", lou_getProgramPath(), tableName);
 #else
-  strcpy (trialPath, TABLESDIR);
-  strcat (trialPath, pathEnd);
+  snprintf (trialPath, MAXSTRING-1, "%s%s%s", TABLESDIR, pathEnd, tableName);
 #endif
-  strcat (trialPath, tableName);
   if ((tableFile = fopen (trialPath, "rb")))
     return tableFile;
   return NULL;
-- 
2.13.5

@kirotawa
Copy link
Author

kirotawa commented Nov 2, 2017

:) thanks a million carnil!!!

@sthibaul
Copy link
Contributor

sthibaul commented Nov 3, 2017

Hello,
This seems a bit bogus to me:

char path[MAXSTRING];
strncpy (trialPath, pathList, k);

nothing makes sure that k is less than MAXSTRING (k is the length of a string coming from the environment).

@sthibaul
Copy link
Contributor

sthibaul commented Nov 3, 2017

Also, the strncpy call doesn't actually put a \0...

@sthibaul
Copy link
Contributor

sthibaul commented Nov 3, 2017

I'll propose the attached patch as fix for Debian on top of this one.

CVE-2014-8184-fix.txt

@egli
Copy link
Member

egli commented Nov 3, 2017

TBH, I'm a bit confused now. I thought the code that had the CVE doesn't even exist in liblouis master anymore. So why is there a patch needed?

@sthibaul
Copy link
Contributor

sthibaul commented Nov 3, 2017

I'm not proposing the patch for upstream liblouis, I'm just putting here as a reference for people who are still distributing that old version of liblouis.

@egli
Copy link
Member

egli commented Nov 3, 2017

So in other words we can close this issue?

@sthibaul
Copy link
Contributor

sthibaul commented Nov 3, 2017

I'd say so, yes

@egli
Copy link
Member

egli commented Nov 3, 2017

OK, thanks :-)

@egli egli closed this as completed Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants