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

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

Share & tools

James Prestwood

author

2019-10-17 15:49 UTC #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

Denis Kenzior

2019-10-17 16:04 UTC #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

James Prestwood

author

2019-10-17 16:11 UTC #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