Repository: nginx/njs (c)

Issue:
NJS 0.8.6 Buffer.indexOf reads out of bounds instead of returning -1.
### Describe the bug

NJS since version 0.8.6 Buffer.indexOf reads out of bounds instead of returning -1.

Before submitting a bug report, please check the following:

- [x] The bug is reproducible with the latest version of njs.
- [x] I minimized the code and NGINX configuration to the smallest
possible to reproduce the issue.

### To reproduce


Steps to reproduce the behavior:
Use the following script in a js_body_filter to replace contents in the data
- JS script
```js
function bufferReplace(data, search, replacement) {
    for (var i = data.indexOf(search, 0); i !== -1; i = data.indexOf(search, i + search.length)) {
        // replace search with replacement in buffer
        replacement.copy(data, i, 0);
    }
}

function fix_response_body(r, data, flags) {
    var data_buffer = typeof data === "string" ? Buffer.from(data) : data;
    bufferReplace(data_buffer, "Text", Buffer.from("Test"));
    r.sendBuffer(data_buffer, flags);
}

export default { fix_response_body }
```

- NGINX configuration
```
js_body_filter somefile.fix_response_body  buffer_type=buffer;
```

- NGINX logs
in case out of bounds read crashes the worker (it does not find "Text" in any of the following memory)
```
nginx-1          | 2024/10/09 05:25:41 [notice] 76#76: signal 17 (SIGCHLD) received from 77
nginx-1          | 2024/10/09 05:25:41 [alert] 76#76: worker process 77 exited on signal 11 (core dumped)
nginx-1          | 2024/10/09 05:25:41 [notice] 76#76: start worker process 332
```
in case out of bounds read finds "Text" in the following memory before it crashes the worker it reports an out of bounds write at `replacement.copy`

### Expected behavior

Buffer.indexOf should read -1 when reaching the end of the buffer. It should not read out of bounds.

### Your environment

- Version of njs: `ii  nginx-module-njs 1.26.2+0.8.6-1~bookworm amd64        nginx njs dynamic modules`
- Version of NGINX:  `ii  nginx          1.26.2-1~bookworm amd64        high performance web server`
- OS: Debian Bookworm

### Additional context

This is especially dangerous because 0.8.5 did not read out of bounds in this case and if somebody just updates their packages and doesn't test their njs scripts this can lead to workers crashing or worse if the script does handle user input (ours does not).

Initially reported as a security vulnerability but was told that this is not a security vulnerability by the F5 expert.


Discussion:
Hi @ap-wtioit,

Thank you for the report. I was able to reproduce the issue, and I am working on it.



Patch:
diff --git a/src/njs_buffer.c b/src/njs_buffer.c
--- a/src/njs_buffer.c
+++ b/src/njs_buffer.c
@@ -2117,10 +2117,6 @@ njs_buffer_prototype_index_of(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
 
     index = -1;
 
-    if (njs_slow_path(array->byte_length == 0)) {
-        goto done;
-    }
-
     length = array->byte_length;
 
     if (last) {
@@ -2145,30 +2141,11 @@ njs_buffer_prototype_index_of(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
             return ret;
         }
 
-        if (last) {
-            if (from >= 0) {
-                from = njs_min(from, length - 1);
-
-            } else if (from < 0) {
-                from += length;
-            }
-
-            if (from <= to) {
-                goto done;
-            }
+        if (from >= 0) {
+            from = njs_min(from, length);
 
         } else {
-            if (from < 0) {
-                from += length;
-
-                if (from < 0) {
-                    from = 0;
-                }
-            }
-
-            if (from >= to) {
-                goto done;
-            }
+            from = njs_max(0, length + from);
         }
     }
 
@@ -2213,22 +2190,24 @@ njs_buffer_prototype_index_of(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
             str.length = src->byte_length;
         }
 
-        if (njs_slow_path(str.length == 0)) {
-            index = (last) ? length : 0;
-            goto done;
-        }
-
-        if (str.length > (size_t) length) {
-            goto done;
-        }
-
         if (last) {
-            from -= str.length - 1;
-            from = njs_max(from, 0);
+            from = njs_min(from, length - (int64_t) str.length);
+
+            if (to > from) {
+                goto done;
+            }
 
         } else {
             to -= str.length - 1;
-            to = njs_min(to, length);
+
+            if (from > to) {
+                goto done;
+            }
+        }
+
+        if (from == to && str.length == 0) {
+            index = 0;
+            goto done;
         }
 
         for (i = from; i != to; i += increment) {
@@ -2243,6 +2222,10 @@ njs_buffer_prototype_index_of(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     case NJS_NUMBER:
         byte = njs_number_to_uint32(njs_number(value));
 
+        if (last) {
+            from = njs_min(from, length - 1);
+        }
+
         for (i = from; i != to; i += increment) {
             if (u8[i] == byte) {
                 index = i;
diff --git a/src/qjs_buffer.c b/src/qjs_buffer.c
--- a/src/qjs_buffer.c
+++ b/src/qjs_buffer.c
@@ -988,10 +988,6 @@ qjs_buffer_prototype_index_of(JSContext *ctx, JSValueConst this_val, int argc,
 
     length = self.length;
 
-    if (length == 0) {
-        return JS_NewInt32(ctx, -1);
-    }
-
     if (last) {
         from = length - 1;
         to = -1;
@@ -1015,30 +1011,11 @@ qjs_buffer_prototype_index_of(JSContext *ctx, JSValueConst this_val, int argc,
             return JS_EXCEPTION;
         }
 
-        if (last) {
-            if (from >= 0) {
-                from = njs_min(from, length - 1);
-
-            } else if (from < 0) {
-                from += length;
-            }
-
-            if (from <= to) {
-                return JS_NewInt32(ctx, -1);
-            }
+        if (from >= 0) {
+            from = njs_min(from, length);
 
         } else {
-            if (from < 0) {
-                from += length;
-
-                if (from < 0) {
-                    from = 0;
-                }
-            }
-
-            if (from >= to) {
-                return JS_NewInt32(ctx, -1);
-            }
+            from = njs_max(0, length + from);
         }
     }
 
@@ -1047,6 +1024,10 @@ qjs_buffer_prototype_index_of(JSContext *ctx, JSValueConst this_val, int argc,
             return JS_EXCEPTION;
         }
 
+        if (last) {
+            from = njs_min(from, length - 1);
+        }
+
         for (i = from; i != to; i += increment) {
             if (self.start[i] == (uint8_t) byte) {
                 return JS_NewInt32(ctx, i);
@@ -1082,23 +1063,24 @@ qjs_buffer_prototype_index_of(JSContext *ctx, JSValueConst this_val, int argc,
                                 "or Buffer-like object");
     }
 
-    if (str.length == 0) {
-        JS_FreeValue(ctx, buffer);
-        return JS_NewInt32(ctx, (last) ? length : 0);
-    }
-
-    if (str.length > (size_t) length) {
-        JS_FreeValue(ctx, buffer);
-        return JS_NewInt32(ctx, -1);
-    }
-
     if (last) {
-        from -= str.length - 1;
-        from = njs_max(from, 0);
+        from = njs_min(from, length - (int64_t) str.length);
+
+        if (to > from) {
+            goto done;
+        }
 
     } else {
         to -= str.length - 1;
-        to = njs_min(to, length);
+
+        if (from > to) {
+            goto done;
+        }
+    }
+
+    if (from == to && str.length == 0) {
+        JS_FreeValue(ctx, buffer);
+        return JS_NewInt32(ctx, 0);
     }
 
     for (i = from; i != to; i += increment) {
@@ -1108,6 +1090,8 @@ qjs_buffer_prototype_index_of(JSContext *ctx, JSValueConst this_val, int argc,
         }
     }
 
+done:
+
     JS_FreeValue(ctx, buffer);
     return JS_NewInt32(ctx, -1);
 }


Test patch:
diff --git a/test/buffer.t.js b/test/buffer.t.js
--- a/test/buffer.t.js
+++ b/test/buffer.t.js
@@ -473,6 +473,20 @@ let indexOf_tsuite = {
         { buf: Buffer.from('abcdef'), value: 'abc', offset: 1, expected: -1 },
         { buf: Buffer.from('abcdef'), value: 'def', offset: 1, expected: 3 },
         { buf: Buffer.from('abcdef'), value: 'def', offset: -3, expected: 3 },
+        { buf: Buffer.from('abcdef'), value: 'efgh', offset: 4, expected: -1 },
+        { buf: Buffer.from(''), value: '', expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: -1, expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: 0, expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: 1, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -4, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -3, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -2, expected: 1 },
+        { buf: Buffer.from('abc'), value: '', offset: -1, expected: 2 },
+        { buf: Buffer.from('abc'), value: '', offset: 0, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: 1, expected: 1 },
+        { buf: Buffer.from('abc'), value: '', offset: 2, expected: 2 },
+        { buf: Buffer.from('abc'), value: '', offset: 3, expected: 3 },
+        { buf: Buffer.from('abc'), value: '', offset: 4, expected: 3 },
         { buf: Buffer.from('abcdef'), value: '626364', encoding: 'hex', expected: 1 },
         { buf: Buffer.from('abcdef'), value: '626364', encoding: 'utf-128',
           exception: 'TypeError: "utf-128" encoding is not supported' },
@@ -582,6 +596,20 @@ let lastIndexOf_tsuite = {
         { buf: Buffer.from('abcdef'), value: 'def', expected: 3 },
         { buf: Buffer.from('abcdef'), value: 'abc', offset: 1, expected: 0 },
         { buf: Buffer.from('abcdef'), value: 'def', offset: 1, expected: -1 },
+        { buf: Buffer.from('xxxABCx'), value: 'ABC', offset: 3, expected: 3 },
+        { buf: Buffer.from(''), value: '', expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: -1, expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: 0, expected: 0 },
+        { buf: Buffer.from(''), value: '', offset: 1, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -4, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -3, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: -2, expected: 1 },
+        { buf: Buffer.from('abc'), value: '', offset: -1, expected: 2 },
+        { buf: Buffer.from('abc'), value: '', offset: 0, expected: 0 },
+        { buf: Buffer.from('abc'), value: '', offset: 1, expected: 1 },
+        { buf: Buffer.from('abc'), value: '', offset: 2, expected: 2 },
+        { buf: Buffer.from('abc'), value: '', offset: 3, expected: 3 },
+        { buf: Buffer.from('abc'), value: '', offset: 4, expected: 3 },
         { buf: Buffer.from(Buffer.alloc(7).fill('Zabcdef').buffer, 1), value: 'abcdef', expected: 0 },
         { buf: Buffer.from(Buffer.alloc(7).fill('Zabcdef').buffer, 1), value: 'abcdefg', expected: -1 },
         { buf: Buffer.from('abcdef'), value: '626364', encoding: 'hex', expected: 1 },
@@ -589,6 +617,7 @@ let lastIndexOf_tsuite = {
           exception: 'TypeError: "utf-128" encoding is not supported' },
         { buf: Buffer.from('abcabc'), value: 0x61, expected: 3 },
         { buf: Buffer.from('abcabc'), value: 0x61, offset: 1, expected: 0 },
+        { buf: Buffer.from('ab'), value: 7, offset: 2, expected: -1 },
         { buf: Buffer.from('abcdef'), value: Buffer.from('def'), expected: 3 },
         { buf: Buffer.from('abcdef'), value: Buffer.from(new Uint8Array([0x60, 0x62, 0x63]).buffer, 1), expected: 1 },
         { buf: Buffer.from('abcdef'), value: {},
diff --git a/test/harness/runTsuite.js b/test/harness/runTsuite.js
--- a/test/harness/runTsuite.js
+++ b/test/harness/runTsuite.js
@@ -30,7 +30,7 @@ async function run(tlist) {
         let r = results.map((r, i) => validate(ts.tests, r, i));
 
         r.forEach((v, i) => {
-            assert.sameValue(v, true, `FAILED ${i}: ${JSON.stringify(ts.tests[i])}\n    with reason: ${results[i].reason}`);
+            assert.sameValue(v, true, `FAILED ${ts.name}: ${JSON.stringify(ts.tests[i])}\n    with reason: ${results[i].reason}`);
         })
     }
 }