Fix GH-13984: Buffer size is now checked before memcmp #13991

SakiTakamachi · 2024-04-17T15:45:46Z

This was also reproduced in 8.2.
Can the push test detect overflow?

SakiTakamachi · 2024-04-17T15:58:59Z

ext/pdo_sqlite/sqlite_driver.c

@@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
 		}
 		return estrdup(filename);
 	}
-	if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
+	if (*filename && (sizeof(filename) != sizeof(":memory:") || memcmp(filename, ":memory:", sizeof(":memory:")) != 0)) {


)))) was too hard to see, so I added != 0. The meaning remains the same.

This is wrong, sizeof(filename) is not the length, it is the size of the pointer.
I also think the != 0 addition is wrong for memcmp?

why bothering with memcmp anyway ? why not just any str*cmp ?

@nielsdos

This is wrong, sizeof(filename) is not the length, it is the size of the pointer.

Right. I entered sizeof with the intention of strlen...

I also think the != 0 addition is wrong for memcmp?

Since it becomes true when 1 and -1, it seems to be the same because it becomes true when != 0, but am I wrong?

@devnexen

why bothering with memcmp anyway ? why not just any str*cmp ?

I thought I had to convert it to zend_string, but there were some that could be used as char, so I fixed it.

Right, I misread the original condition. So !=0 is fine.

SakiTakamachi · 2024-04-17T16:01:46Z

ext/pdo_sqlite/tests/gh13991.phpt

+pdo_sqlite
+--FILE--
+<?php
+$dbfile = 'z.db';


I did this because specifying the absolute path would make the string longer and not be overloaded.

Hm right... This can clash with local files though, so perhaps it's better to use 13991 as filename?

SakiTakamachi · 2024-04-17T16:02:46Z

ext/pdo_sqlite/tests/gh13991.phpt

+?>
+--CLEAN--
+<?php
+@unlink(getcwd() . '/z.db');


Is there a better way?

We should be careful somehow that we don't destroy files or users running the tests.

Changed to skip if a file with the same name already exists.

nielsdos · 2024-04-17T16:04:09Z

ext/pdo_sqlite/sqlite_driver.c

@@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
 		}
 		return estrdup(filename);
 	}
-	if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
+	if (*filename && (sizeof(filename) != sizeof(":memory:") || memcmp(filename, ":memory:", sizeof(":memory:")) != 0)) {


This is wrong, sizeof(filename) is not the length, it is the size of the pointer.
I also think the != 0 addition is wrong for memcmp?

nielsdos · 2024-04-17T16:05:01Z

ext/pdo_sqlite/tests/gh13991.phpt

+pdo_sqlite
+--FILE--
+<?php
+$dbfile = 'z.db';


Hm right... This can clash with local files though, so perhaps it's better to use 13991 as filename?

nielsdos · 2024-04-17T16:05:27Z

ext/pdo_sqlite/tests/gh13991.phpt

+?>
+--CLEAN--
+<?php
+@unlink(getcwd() . '/z.db');


We should be careful somehow that we don't destroy files or users running the tests.

nielsdos · 2024-04-17T16:06:21Z

Can the push test detect overflow?

No, it seems ASAN isn't ran on this branch on push.

devnexen · 2024-04-17T16:40:47Z

ext/pdo_sqlite/sqlite_driver.c

@@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
 		}
 		return estrdup(filename);
 	}
-	if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
+	if (*filename && zend_binary_strcmp(filename, strlen(filename)-1, ":memory:", strlen(":memory:")-1)) {


I know very little pdo sqlite, but I do not get why -1 for the 2 strlen ? Legit question :)

nielsdos · 2024-04-17T16:40:57Z

ext/pdo_sqlite/sqlite_driver.c

@@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
 		}
 		return estrdup(filename);
 	}
-	if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
+	if (*filename && zend_binary_strcmp(filename, strlen(filename)-1, ":memory:", strlen(":memory:")-1)) {


Subtracting 1 from strlen is not necessary, the subtraction of 1 from sizeof is because sizeof also counts the NULL terminator but strlen does not.

I was thinking about that same kind of confusion, just wanted to confirm.

It seems like I'm a little too sleepy....Sorry for making me check again and again. (AM 1:43 JST)

it s ok Saki :) relax, as nielsdos mentioned, an "attacker" can t do anything really malicious here. no rush :)

nielsdos · 2024-04-17T17:14:54Z

ext/pdo_sqlite/sqlite_driver.c

@@ -751,7 +751,7 @@ static char *make_filename_safe(const char *filename)
 		}
 		return estrdup(filename);
 	}
-	if (*filename && memcmp(filename, ":memory:", sizeof(":memory:"))) {
+	if (*filename && zend_binary_strcmp(filename, strlen(filename), ":memory:", strlen(":memory:"))) {


I think I prefer just a regular call to strcmp, simpler and prevents computing the length too.
The binary strcmp doesn't rlly have an advantage when using strlen.

Changed it :)

devnexen

LCTM, at least the code part :)

nielsdos

Tested locally, test correctly reproduces the issue and patch seems right.
Thanks Saki.

SakiTakamachi · 2024-04-17T17:36:09Z

Thanks for the check. I'll merge it tomorrow!

* PHP-8.2: Fix GH-13984: Buffer size is now checked before memcmp (#13991)

* PHP-8.3: Fix GH-13984: Buffer size is now checked before memcmp (#13991)

github-actions bot added the Extension: pdo_sqlite label Apr 17, 2024

SakiTakamachi force-pushed the fix/gh-13984 branch from 94cfc0d to 6e41b7d Compare April 17, 2024 15:52

SakiTakamachi marked this pull request as ready for review April 17, 2024 15:53

SakiTakamachi requested review from nielsdos and devnexen April 17, 2024 15:55

SakiTakamachi changed the base branch from master to PHP-8.2 April 17, 2024 15:56

Buffer size is now checked before memcmp

e93c35b

SakiTakamachi force-pushed the fix/gh-13984 branch from 6e41b7d to e93c35b Compare April 17, 2024 15:56

SakiTakamachi commented Apr 17, 2024

View reviewed changes

nielsdos requested changes Apr 17, 2024

View reviewed changes

address comments

28a51dc

devnexen reviewed Apr 17, 2024

View reviewed changes

nielsdos requested changes Apr 17, 2024

View reviewed changes

SakiTakamachi added 2 commits April 18, 2024 01:42

fixed test

15e34dd

remove -1

d248a60

nielsdos reviewed Apr 17, 2024

View reviewed changes

use strcmp

5b64ffe

devnexen approved these changes Apr 17, 2024

View reviewed changes

nielsdos approved these changes Apr 17, 2024

View reviewed changes

SakiTakamachi added a commit that referenced this pull request Apr 17, 2024

Merge branch 'PHP-8.2' into PHP-8.3

dd5cdfd

* PHP-8.2: Fix GH-13984: Buffer size is now checked before memcmp (#13991)

SakiTakamachi closed this in fda91a0 Apr 17, 2024

SakiTakamachi added a commit that referenced this pull request Apr 17, 2024

Merge branch 'PHP-8.3'

d14b208

* PHP-8.3: Fix GH-13984: Buffer size is now checked before memcmp (#13991)

SakiTakamachi deleted the fix/gh-13984 branch June 13, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-13984: Buffer size is now checked before memcmp #13991

Fix GH-13984: Buffer size is now checked before memcmp #13991

SakiTakamachi commented Apr 17, 2024 •

edited

Loading

SakiTakamachi Apr 17, 2024

nielsdos Apr 17, 2024

devnexen Apr 17, 2024

SakiTakamachi Apr 17, 2024

nielsdos Apr 17, 2024

SakiTakamachi Apr 17, 2024 •

edited

Loading

nielsdos Apr 17, 2024

SakiTakamachi Apr 17, 2024

nielsdos Apr 17, 2024

SakiTakamachi Apr 17, 2024

nielsdos Apr 17, 2024

nielsdos Apr 17, 2024

nielsdos Apr 17, 2024

nielsdos commented Apr 17, 2024

devnexen Apr 17, 2024

nielsdos Apr 17, 2024

devnexen Apr 17, 2024

SakiTakamachi Apr 17, 2024

devnexen Apr 17, 2024

nielsdos Apr 17, 2024

SakiTakamachi Apr 17, 2024

devnexen left a comment

nielsdos left a comment

SakiTakamachi commented Apr 17, 2024 •

edited

Loading

Fix GH-13984: Buffer size is now checked before memcmp #13991

Fix GH-13984: Buffer size is now checked before memcmp #13991

Conversation

SakiTakamachi commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen left a comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Apr 17, 2024 • edited Loading

SakiTakamachi commented Apr 17, 2024 •

edited

Loading

SakiTakamachi Apr 17, 2024 •

edited

Loading

SakiTakamachi commented Apr 17, 2024 •

edited

Loading