diff mbox

[U-Boot,1/2] FIX: env: import: hashtable: Prevent buffer overrun when importing environment from file

Message ID 1442185024-14645-1-git-send-email-l.majewski@majess.pl
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Lukasz Majewski Sept. 13, 2015, 10:57 p.m. UTC
Lets consider following scenario:
- One uses echo -n "key=value" to define environment variable in a file (single variable)
- The file content is "key=value" without any terminating byte (e.g. 0x0a or
0x0d).
- The file is loaded to u-boot non zero'ed RAM buffer (with load command).
- Then "env import -t -r $loadaddr $filesize" is executed.
- Due to lack of proper termination byte we have classical example of buffer
  overrun.

This patch prevents from this by allocating one extra byte than size and
explicitly null terminate it.

There should be no change for normal env import operation after applying
this patch.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
 lib/hashtable.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Tom Rini Sept. 15, 2015, 9:34 p.m. UTC | #1
On Mon, Sep 14, 2015 at 12:57:03AM +0200, Lukasz Majewski wrote:

> Lets consider following scenario:
> - One uses echo -n "key=value" to define environment variable in a file (single variable)
> - The file content is "key=value" without any terminating byte (e.g. 0x0a or
> 0x0d).
> - The file is loaded to u-boot non zero'ed RAM buffer (with load command).
> - Then "env import -t -r $loadaddr $filesize" is executed.
> - Due to lack of proper termination byte we have classical example of buffer
>   overrun.
> 
> This patch prevents from this by allocating one extra byte than size and
> explicitly null terminate it.
> 
> There should be no change for normal env import operation after applying
> this patch.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

Applied to u-boot/master, thanks!
diff mbox

Patch

diff --git a/lib/hashtable.c b/lib/hashtable.c
index 18ed590..7df424a 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -789,12 +789,13 @@  int himport_r(struct hsearch_data *htab,
 	}
 
 	/* we allocate new space to make sure we can write to the array */
-	if ((data = malloc(size)) == NULL) {
-		debug("himport_r: can't malloc %zu bytes\n", size);
+	if ((data = malloc(size + 1)) == NULL) {
+		debug("himport_r: can't malloc %zu bytes\n", size + 1);
 		__set_errno(ENOMEM);
 		return 0;
 	}
 	memcpy(data, env, size);
+	data[size] = '\0';
 	dp = data;
 
 	/* make a local copy of the list of variables */