Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commita92db3d

Browse files
committed
Fix PQescapeLiteral()/PQescapeIdentifier() length handling
In5dc1e42 I fixed bugs in various escape functions, unfortunately as partof that I introduced a new bug in PQescapeLiteral()/PQescapeIdentifier(). Thebug is that I made PQescapeInternal() just use strlen(), rather than takingthe specified input length into account.That's bad, because it can lead to including input that wasn't intended to beincluded (in case len is shorter than null termination of the string) andbecause it can lead to reading invalid memory if the input string is not nullterminated.Expand test_escape to this kind of bug:a) for escape functions with length support, append data that should not be escaped and check that it is notb) add valgrind requests to detect access of bytes that should not be touchedAuthor: Tom Lane <tgl@sss.pgh.pa.us>Author: Andres Freund <andres@anarazel.deReviewed-by: Tom Lane <tgl@sss.pgh.pa.us>Reviewed-by: Noah Misch <noah@leadboat.com>Discussion:https://postgr.es/m/Z64jD3u46gObCo1p@pryzbyj2023Backpatch: 13
1 parent113fc65 commita92db3d

File tree

2 files changed

+77
-5
lines changed

2 files changed

+77
-5
lines changed

‎src/interfaces/libpq/fe-exec.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4224,7 +4224,7 @@ PQescapeInternal(PGconn *conn, const char *str, size_t len, bool as_ident)
42244224
char*rp;
42254225
intnum_quotes=0;/* single or double, depending on as_ident */
42264226
intnum_backslashes=0;
4227-
size_tinput_len=strlen(str);
4227+
size_tinput_len=strnlen(str,len);
42284228
size_tresult_size;
42294229
charquote_char=as_ident ?'"' :'\'';
42304230
boolvalidated_mb= false;
@@ -4274,7 +4274,7 @@ PQescapeInternal(PGconn *conn, const char *str, size_t len, bool as_ident)
42744274
if (!validated_mb)
42754275
{
42764276
if (pg_encoding_verifymbstr(conn->client_encoding,s,remaining)
4277-
!=strlen(s))
4277+
!=remaining)
42784278
{
42794279
libpq_append_conn_error(conn,"invalid multibyte character");
42804280
returnNULL;

‎src/test/modules/test_escape/test_escape.c

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include"getopt_long.h"
1818
#include"libpq-fe.h"
1919
#include"mb/pg_wchar.h"
20+
#include"utils/memdebug.h"
2021

2122

2223
typedefstructpe_test_config
@@ -56,6 +57,11 @@ typedef struct pe_test_escape_func
5657
*/
5758
boolsupports_only_ascii_overlap;
5859

60+
/*
61+
* Does the escape function have a length input?
62+
*/
63+
boolsupports_input_length;
64+
5965
bool(*escape) (PGconn*conn,PQExpBuffertarget,
6066
constchar*unescaped,size_tunescaped_len,
6167
PQExpBufferescape_err);
@@ -234,28 +240,33 @@ static pe_test_escape_func pe_test_escape_funcs[] =
234240
{
235241
.name="PQescapeLiteral",
236242
.reports_errors= true,
243+
.supports_input_length= true,
237244
.escape=escape_literal,
238245
},
239246
{
240247
.name="PQescapeIdentifier",
241248
.reports_errors= true,
249+
.supports_input_length= true,
242250
.escape=escape_identifier
243251
},
244252
{
245253
.name="PQescapeStringConn",
246254
.reports_errors= true,
255+
.supports_input_length= true,
247256
.escape=escape_string_conn
248257
},
249258
{
250259
.name="PQescapeString",
251260
.reports_errors= false,
261+
.supports_input_length= true,
252262
.escape=escape_string
253263
},
254264
{
255265
.name="replace",
256266
.reports_errors= false,
257267
.supports_only_valid= true,
258268
.supports_only_ascii_overlap= true,
269+
.supports_input_length= true,
259270
.escape=escape_replace
260271
},
261272
{
@@ -272,6 +283,7 @@ static pe_test_escape_func pe_test_escape_funcs[] =
272283

273284

274285
#defineTV(enc,string) {.client_encoding = (enc), .escape=string, .escape_len=sizeof(string) - 1, }
286+
#defineTV_LEN(enc,string,len) {.client_encoding = (enc), .escape=string, .escape_len=len, }
275287
staticpe_test_vectorpe_test_vectors[]=
276288
{
277289
/* expected to work sanity checks */
@@ -359,6 +371,15 @@ static pe_test_vector pe_test_vectors[] =
359371
TV("mule_internal","\\\x9c';\0;"),
360372

361373
TV("sql_ascii","1\xC0'"),
374+
375+
/*
376+
* Testcases that are not null terminated for the specified input length.
377+
* That's interesting to verify that escape functions don't read beyond
378+
* the intended input length.
379+
*/
380+
TV_LEN("gbk","\x80",1),
381+
TV_LEN("UTF-8","\xC3\xb6 ",1),
382+
TV_LEN("UTF-8","\xC3\xb6 ",2),
362383
};
363384

364385

@@ -521,6 +542,7 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
521542
{
522543
PQExpBuffertestname;
523544
PQExpBufferdetails;
545+
PQExpBufferraw_buf;
524546
PQExpBufferescape_buf;
525547
PQExpBufferescape_err;
526548
size_tinput_encoding_validlen;
@@ -534,6 +556,7 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
534556
escape_err=createPQExpBuffer();
535557
testname=createPQExpBuffer();
536558
details=createPQExpBuffer();
559+
raw_buf=createPQExpBuffer();
537560
escape_buf=createPQExpBuffer();
538561

539562
if (ef->supports_only_ascii_overlap&&
@@ -567,8 +590,8 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
567590

568591
input_encoding0_validlen=pg_encoding_verifymbstr(PQclientEncoding(tc->conn),
569592
tv->escape,
570-
strlen(tv->escape));
571-
input_encoding0_valid=input_encoding0_validlen==strlen(tv->escape);
593+
strnlen(tv->escape,tv->escape_len));
594+
input_encoding0_valid=input_encoding0_validlen==strnlen(tv->escape,tv->escape_len);
572595
appendPQExpBuffer(details,"#\t input encoding valid till 0: %d\n",
573596
input_encoding0_valid);
574597

@@ -580,9 +603,45 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
580603
gotoout;
581604

582605

606+
/*
607+
* Put the to-be-escaped data into a buffer, so that we
608+
*
609+
* a) can mark memory beyond end of the string as inaccessible when using
610+
* valgrind
611+
*
612+
* b) can append extra data beyond the length passed to the escape
613+
* function, to verify that that data is not processed.
614+
*
615+
* TODO: Should we instead/additionally escape twice, once with unmodified
616+
* and once with appended input? That way we could compare the two.
617+
*/
618+
appendBinaryPQExpBuffer(raw_buf,tv->escape,tv->escape_len);
619+
620+
#defineNEVER_ACCESS_STR "\xff never-to-be-touched"
621+
if (ef->supports_input_length)
622+
{
623+
/*
624+
* Append likely invalid string that does *not* contain a null byte
625+
* (which'd prevent some invalid accesses to later memory).
626+
*/
627+
appendPQExpBufferStr(raw_buf,NEVER_ACCESS_STR);
628+
629+
VALGRIND_MAKE_MEM_NOACCESS(&raw_buf->data[tv->escape_len],
630+
raw_buf->len-tv->escape_len);
631+
}
632+
else
633+
{
634+
/* append invalid string, after \0 */
635+
appendPQExpBufferChar(raw_buf,0);
636+
appendPQExpBufferStr(raw_buf,NEVER_ACCESS_STR);
637+
638+
VALGRIND_MAKE_MEM_NOACCESS(&raw_buf->data[tv->escape_len+1],
639+
raw_buf->len-tv->escape_len-1);
640+
}
641+
583642
/* call the to-be-tested escape function */
584643
escape_success=ef->escape(tc->conn,escape_buf,
585-
tv->escape,tv->escape_len,
644+
raw_buf->data,tv->escape_len,
586645
escape_err);
587646
if (!escape_success)
588647
{
@@ -592,6 +651,8 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
592651

593652
if (escape_buf->len>0)
594653
{
654+
boolcontains_never;
655+
595656
appendPQExpBuffer(details,"#\t escaped string: %zd bytes: ",escape_buf->len);
596657
escapify(details,escape_buf->data,escape_buf->len);
597658
appendPQExpBufferChar(details,'\n');
@@ -603,6 +664,16 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
603664

604665
appendPQExpBuffer(details,"#\t escape encoding valid: %d\n",
605666
escape_encoding_valid);
667+
668+
/*
669+
* Verify that no data beyond the end of the input is included in the
670+
* escaped string. It'd be better to use something like memmem()
671+
* here, but that's not available everywhere.
672+
*/
673+
contains_never=strstr(escape_buf->data,NEVER_ACCESS_STR)==NULL;
674+
report_result(tc,contains_never,testname,details,
675+
"escaped data beyond end of input",
676+
contains_never ?"no" :"all secrets revealed");
606677
}
607678
else
608679
{
@@ -693,6 +764,7 @@ test_one_vector_escape(pe_test_config *tc, const pe_test_vector *tv, const pe_te
693764
destroyPQExpBuffer(details);
694765
destroyPQExpBuffer(testname);
695766
destroyPQExpBuffer(escape_buf);
767+
destroyPQExpBuffer(raw_buf);
696768
}
697769

698770
staticvoid

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp