Skip to contentSkip to conversation

util: add bounds check to util_get_{domain,username}

PATCH Superseded James Prestwood 2 participants 3 messages Oct 17, 2019

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

James Prestwood

author
#0 lore raw

PATCH v2 1/1 util: add bounds check to util_get_{domain,username}

Two cases were using strcpy, and the other two were using strncpy. Instead all cases can use l_strlcpy which guarentees NULL termination.

---
 src/util.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
src/util.c +4 -4
index f787ce6b..1291d9c9 100644
@@ -173,10 +173,10 @@ const char *util_get_domain(const char *identity)
 	for (c = identity; *c; c++) {
 		switch (*c) {
 		case '\\':
-			strncpy(domain, identity, c - identity);
+			l_strlcpy(domain, identity, c - identity);
 			return domain;
 		case '@':
-			strcpy(domain, c + 1);
+			l_strlcpy(domain, c + 1, sizeof(domain));
 			return domain;
 		default:
 			continue;

@@ -197,10 +197,10 @@ const char *util_get_username(const char *identity)
 	for (c = identity; *c; c++) {
 		switch (*c) {
 		case '\\':
-			strcpy(username, c + 1);
+			l_strlcpy(username, c + 1, sizeof(username));
 			return username;
 		case '@':
-			strncpy(username, identity, c - identity);
+			l_strlcpy(username, identity, c - identity);
 			return username;
 		default:
 			continue;
Signature · 1 line
2.17.1
#1 lore raw

Hi James,

Quoted 11 lines · James Prestwood

Two cases were using strcpy, and the other two were using strncpy. Instead all cases can use l_strlcpy which guarentees NULL termination.

---
  src/util.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
src/util.c +0 -0
index f787ce6b..1291d9c9 100644

src/util.c const char *util_get_domain(const char *identity) · line 176

Quoted 5 lines
src/util.c +1 -1
@@ -173,10 +173,10 @@ const char *util_get_domain(const char *identity)
  	for (c = identity; *c; c++) {
  		switch (*c) {
  		case '\\':
-			strncpy(domain, identity, c - identity);
+			l_strlcpy(domain, identity, c - identity);

Using l_strlcpy here is wrong. You want to copy a substrict of c - identity bytes into domain where identity does not have any embedded nulls. But passing c - identity into l_strlcpy would not work the way
you think. Also, the existing code has an inherent assumption that c -
identity is always <= 253 bytes. So really, nothing here is dangerous...

If anything we should be using memcpy to make this more clear. If you want to be pedantic, then you need to check that c - identity is < sizeof(domain).

src/util.c const char *util_get_domain(const char *identity) · line 179

Quoted 4 lines
src/util.c +1 -1
  			return domain;
  		case '@':
-			strcpy(domain, c + 1);
+			l_strlcpy(domain, c + 1, sizeof(domain));

Here we do have a terminating NULL, so using l_strlcpy is fine.

src/util.c const char *util_get_username(const char *identity) · line 200

Quoted 9 lines
src/util.c +1 -1
  			return domain;
  		default:
  			continue;

@@ -197,10 +197,10 @@ const char *util_get_username(const char *identity)
  	for (c = identity; *c; c++) {
  		switch (*c) {
  		case '\\':
-			strcpy(username, c + 1);
+			l_strlcpy(username, c + 1, sizeof(username));

as above

src/util.c const char *util_get_username(const char *identity) · line 203

Quoted 4 lines
src/util.c +1 -1
  			return username;
  		case '@':
-			strncpy(username, identity, c - identity);
+			l_strlcpy(username, identity, c - identity);

The very first comment applies here as well

src/util.c const char *util_get_username(const char *identity) · line 206

Quoted 4 lines
src/util.c +0 -0
  			return username;
  		default:
  			continue;

Regards,
-Denis

author
#2 lore raw
Quoted 31 lines · Denis Kenzior

Hi James,

Quoted 12 lines · James Prestwood

Two cases were using strcpy, and the other two were using strncpy. Instead all cases can use l_strlcpy which guarentees NULL termination.

---
  src/util.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
src/util.c +0 -0
index f787ce6b..1291d9c9 100644

src/util.c const char *util_get_domain(const char · line 177

Quoted 6 lines
src/util.c +1 -1
@@ -173,10 +173,10 @@ const char *util_get_domain(const char
 *identity)
  	for (c = identity; *c; c++) {
  		switch (*c) {
  		case '\\':
-			strncpy(domain, identity, c - identity);
+			l_strlcpy(domain, identity, c - identity);

Using l_strlcpy here is wrong. You want to copy a substrict of c - identity bytes into domain where identity does not have any embedded nulls. But passing c - identity into l_strlcpy would not work the way
you think. Also, the existing code has an inherent assumption that
c -
identity is always <= 253 bytes. So really, nothing here is dangerous...

Quoted 4 lines

If anything we should be using memcpy to make this more clear. If you
want to be pedantic, then you need to check that c - identity is < sizeof(domain).

Ok, since there is actually a comment explaining the identity length assumption I will just do memcpy and not bother checking the length.

Quoted 7 lines
Quoted 1 line

Regards,
-Denis
_______________________________________________
iwd mailing list -- iwd(a)lists.01.org
To unsubscribe send an email to iwd-leave(a)lists.01.org