On Fri, 27 Oct 2017 11:28:47 +0200
Willy Tarreau <w...@1wt.eu> wrote:
> On Fri, Oct 27, 2017 at 11:19:48AM +0200, Thierry Fournier wrote:
> > It is really sad because the Lua ensure that the final '\0' is set :-( Maybe
> > it will be clever to add a function like regex_prepare_string which copy a
> > const string or not according with the compilation options and the final
> > '\0'
> > found or not ?
>
> It's tricky because you don't know if you're allowed to even *read* this
> trailing zero. Imagine if you have a 16-byte string allocated at 0x123BFF0,
> the zero will be located at 0x123C000 which belongs to another page and may
> or may not be allocated, risking a segfault once in a while.
>
> > Or two parameters for the regex function: "char *subject" and
> > "const char * subject0". These parameters are exclusive, or a flag
> > confirming
> > or not the final '\0'.
>
> Yes that would probably be better. In fact since this read-write subject
> is a pain to deal with from all call places, it would be better that the
> called function duplicates it by itself when needed (ie the match function
> would use a const char* and duplicate the subject for libc). Also people
> using libc's regex don't care much about performance and wil not notice
> the memcpy.
>
> > Note that, I try to save a memcpy(), which use a little bit of CPU, just
> > before
> > executing a regex which heavily use the CPU, maybe it is useless.
>
> Yep. I think the easiest to do *for now* is to perform the memcpy(trash) in
> your patch as it's well isolated and the Lua code doesn't use the trash, and
> then in the future we'll try to improve the regex code to use only const and
> perform the copies itself when appropriate. A bit like we do with the samples
> marked const.
It seems that you push my first version of the patchn so I put a fix in
attachment.
Thierry
> Cheers,
> Willy
>
--
Thierry Fournier
Web Performance & Security Expert
m: +33 6 68 69 21 85 | e: thierry.fourn...@ozon.io
w: http://www.ozon.io/ | b: http://blog.ozon.io/
>From 97235b58ef5a1f0db09f3a8682d03c2fc3481ed3 Mon Sep 17 00:00:00 2001
From: Thierry FOURNIER <thierry.fourn...@ozon.io>
Date: Fri, 27 Oct 2017 14:13:51 +0200
Subject: [PATCH] BUG/MINOR: const attribute of a string is overridden
If HAProxy is compiled without PCRE regexes, this can cause
a write in const memory. The probability of a consequence is
very low.
---
src/hlua_fcn.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
index c37e2a9..a5cae86 100644
--- a/src/hlua_fcn.c
+++ b/src/hlua_fcn.c
@@ -1119,11 +1119,22 @@ static int hlua_regex_exec(struct lua_State *L)
struct my_regex *regex;
const char *str;
size_t len;
+ struct chunk *tmp;
regex = hlua_check_regex(L, 1);
str = luaL_checklstring(L, 2, &len);
- lua_pushboolean(L, regex_exec2(regex, (char *)str, len));
+ /* Copy the string because regex_exec2 require a 'char *'
+ * and not a 'const char *'.
+ */
+ tmp = get_trash_chunk();
+ if (len >= tmp->size) {
+ lua_pushboolean(L, 0);
+ return 1;
+ }
+ memcpy(tmp->str, str, len);
+
+ lua_pushboolean(L, regex_exec2(regex, tmp->str, len));
return 1;
}
@@ -1136,11 +1147,22 @@ static int hlua_regex_match(struct lua_State *L)
regmatch_t pmatch[20];
int ret;
int i;
+ struct chunk *tmp;
regex = hlua_check_regex(L, 1);
str = luaL_checklstring(L, 2, &len);
- ret = regex_exec_match2(regex, (char *)str, len, 20, pmatch, 0);
+ /* Copy the string because regex_exec2 require a 'char *'
+ * and not a 'const char *'.
+ */
+ tmp = get_trash_chunk();
+ if (len >= tmp->size) {
+ lua_pushboolean(L, 0);
+ return 1;
+ }
+ memcpy(tmp->str, str, len);
+
+ ret = regex_exec_match2(regex, tmp->str, len, 20, pmatch, 0);
lua_pushboolean(L, ret);
lua_newtable(L);
if (ret) {
--
2.9.5
