From: stephan Date: Sun, 22 Oct 2023 12:33:05 +0000 (+0000) Subject: JNI: improve UB protections in sqlite3_bind_blob/text/text16(). X-Git-Tag: version-3.44.0~61 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=28f45b1ab1b38b2dbb7800a8978dc00a363a92b3;p=thirdparty%2Fsqlite.git JNI: improve UB protections in sqlite3_bind_blob/text/text16(). FossilOrigin-Name: 5c8383210a87d7f9d37a27053b5b1b6f41794fa8612826c68c1ca49c495cbd97 --- diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c index 8009592b6b..245ce4f9e9 100644 --- a/ext/jni/src/c/sqlite3-jni.c +++ b/ext/jni/src/c/sqlite3-jni.c @@ -841,13 +841,23 @@ static const char * s3jni__jstring_to_mutf8(JNIEnv * const env, jstring v ){ #define s3jni_jstring_to_mutf8(ARG) s3jni__jstring_to_mutf8(env, (ARG)) #define s3jni_mutf8_release(ARG,VAR) if( VAR ) (*env)->ReleaseStringUTFChars(env, ARG, VAR) -static jbyte * s3jni__jbyteArray_bytes(JNIEnv * const env, jbyteArray jBA ){ +/* +** If jBA is not NULL then its GetByteArrayElements() value is +** returned. If jBA is not NULL and nBA is not NULL then *nBA is set +** to the GetArrayLength() of jBA. If GetByteArrayElements() requires +** an allocation and that allocation fails then this function either +** fails fatally or returns 0, depending on build-time options. + */ +static jbyte * s3jni__jbyteArray_bytes2(JNIEnv * const env, jbyteArray jBA, jsize * nBA ){ jbyte * const rv = jBA ? (*env)->GetByteArrayElements(env, jBA, NULL) : 0; s3jni_oom_check( jBA ? !!rv : 1 ); + if( jBA && nBA ) *nBA = (*env)->GetArrayLength(env, jBA); return rv; } -#define s3jni_jbyteArray_bytes(jByteArray) s3jni__jbyteArray_bytes(env, (jByteArray)) +#define s3jni_jbyteArray_bytes2(jByteArray,ptrToSz) \ + s3jni__jbyteArray_bytes2(env, (jByteArray), (ptrToSz)) +#define s3jni_jbyteArray_bytes(jByteArray) s3jni__jbyteArray_bytes2(env, (jByteArray), 0) #define s3jni_jbyteArray_release(jByteArray,jBytes) \ if( jBytes ) (*env)->ReleaseByteArrayElements(env, jByteArray, jBytes, JNI_ABORT) #define s3jni_jbyteArray_commit(jByteArray,jBytes) \ @@ -1010,7 +1020,7 @@ static jstring s3jni__utf8_to_jstring(JNIEnv * const env, static char * s3jni__jstring_to_utf8(JNIEnv * const env, jstring jstr, int *nLen){ jbyteArray jba; - jsize nBa; + jsize nBA; char *rv; if( !jstr ) return 0; @@ -1024,12 +1034,12 @@ static char * s3jni__jstring_to_utf8(JNIEnv * const env, if( nLen ) *nLen = 0; return 0; } - nBa = (*env)->GetArrayLength(env, jba); - if( nLen ) *nLen = (int)nBa; - rv = s3jni_malloc( nBa + 1 ); + nBA = (*env)->GetArrayLength(env, jba); + if( nLen ) *nLen = (int)nBA; + rv = s3jni_malloc( nBA + 1 ); if( rv ){ - (*env)->GetByteArrayRegion(env, jba, 0, nBa, (jbyte*)rv); - rv[nBa] = 0; + (*env)->GetByteArrayRegion(env, jba, 0, nBA, (jbyte*)rv); + rv[nBA] = 0; } S3JniUnrefLocal(jba); return rv; @@ -2359,9 +2369,13 @@ S3JniApi(sqlite3_backup_step(),jint,1backup_1step)( S3JniApi(sqlite3_bind_blob(),jint,1bind_1blob)( JniArgsEnvClass, jlong jpStmt, jint ndx, jbyteArray baData, jint nMax ){ - jbyte * const pBuf = baData ? s3jni_jbyteArray_bytes(baData) : 0; + jsize nBA = 0; + jbyte * const pBuf = baData ? s3jni_jbyteArray_bytes2(baData, &nBA) : 0; int rc; if( pBuf ){ + if( nMax>nBA ){ + nMax = nBA; + } rc = sqlite3_bind_blob(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx, pBuf, (int)nMax, SQLITE_TRANSIENT); s3jni_jbyteArray_release(baData, pBuf); @@ -2448,25 +2462,49 @@ S3JniApi(sqlite3_bind_parameter_name(),jstring,1bind_1parameter_1name)( return z ? s3jni_utf8_to_jstring(z, -1) : 0; } +/* +** Impl of sqlite3_bind_text/text16(). +*/ +static int s3jni__bind_text(int is16, JNIEnv *env, jlong jpStmt, jint ndx, + jbyteArray baData, jint nMax){ + jsize nBA = 0; + jbyte * const pBuf = + baData ? s3jni_jbyteArray_bytes2(baData, &nBA) : 0; + int rc; + if( pBuf ){ + if( nMax>nBA ){ + nMax = nBA; + } + /* Note that we rely on the Java layer having assured that baData + is NUL-terminated if nMax is negative. In order to avoid UB for + such cases, we do not expose the byte-limit arguments in the + public API. */ + rc = is16 + ? sqlite3_bind_text16(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx, + pBuf, (int)nMax, SQLITE_TRANSIENT) + : sqlite3_bind_text(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx, + (const char *)pBuf, + (int)nMax, SQLITE_TRANSIENT); + }else{ + rc = baData + ? sqlite3_bind_null(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx) + : SQLITE_NOMEM; + } + s3jni_jbyteArray_release(baData, pBuf); + return (jint)rc; + +} + S3JniApi(sqlite3_bind_text(),jint,1bind_1text)( JniArgsEnvClass, jlong jpStmt, jint ndx, jbyteArray baData, jint nMax ){ - jbyte * const pBuf = baData ? s3jni_jbyteArray_bytes(baData) : 0; - int const rc = sqlite3_bind_text(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx, - (const char *)pBuf, - (int)nMax, SQLITE_TRANSIENT); - s3jni_jbyteArray_release(baData, pBuf); - return (jint)rc; + return s3jni__bind_text(0, env, jpStmt, ndx, baData, nMax); } S3JniApi(sqlite3_bind_text16(),jint,1bind_1text16)( JniArgsEnvClass, jlong jpStmt, jint ndx, jbyteArray baData, jint nMax ){ - jbyte * const pBuf = baData ? s3jni_jbyteArray_bytes(baData) : 0; - int const rc = sqlite3_bind_text16(S3JniLongPtr_sqlite3_stmt(jpStmt), (int)ndx, - pBuf, (int)nMax, SQLITE_TRANSIENT); - s3jni_jbyteArray_release(baData, pBuf); - return (jint)rc; + return s3jni__bind_text(1, env, jpStmt, ndx, baData, nMax); } S3JniApi(sqlite3_bind_value(),jint,1bind_1value)( @@ -2575,10 +2613,10 @@ S3JniApi(sqlite3_blob_write(),jint,1blob_1write)( ){ sqlite3_blob * const b = S3JniLongPtr_sqlite3_blob(jpBlob); jbyte * const pBuf = b ? s3jni_jbyteArray_bytes(jBa) : 0; - const jsize nBa = pBuf ? (*env)->GetArrayLength(env, jBa) : 0; + const jsize nBA = pBuf ? (*env)->GetArrayLength(env, jBa) : 0; int rc = SQLITE_MISUSE; if(b && pBuf){ - rc = sqlite3_blob_write( b, pBuf, (int)nBa, (int)iOffset ); + rc = sqlite3_blob_write( b, pBuf, (int)nBA, (int)iOffset ); } s3jni_jbyteArray_release(jBa, pBuf); return (jint)rc; @@ -2994,12 +3032,12 @@ S3JniApi(sqlite3_complete(),int,1complete)( JniArgsEnvClass, jbyteArray jSql ){ jbyte * const pBuf = s3jni_jbyteArray_bytes(jSql); - const jsize nBa = pBuf ? (*env)->GetArrayLength(env, jSql) : 0; + const jsize nBA = pBuf ? (*env)->GetArrayLength(env, jSql) : 0; int rc; - assert( (nBa>0 ? 0==pBuf[nBa-1] : (pBuf ? 0==*pBuf : 1)) + assert( (nBA>0 ? 0==pBuf[nBA-1] : (pBuf ? 0==*pBuf : 1)) && "Byte array is not NUL-terminated." ); - rc = (pBuf && 0==pBuf[(nBa ? nBa-1 : 0)]) + rc = (pBuf && 0==pBuf[(nBA ? nBA-1 : 0)]) ? sqlite3_complete( (const char *)pBuf ) : (jSql ? SQLITE_NOMEM : SQLITE_MISUSE); s3jni_jbyteArray_release(jSql, pBuf); @@ -4159,9 +4197,9 @@ static void result_blob_text(int as64 /* true for text64/blob64() mode */, return; }else if( jBa ){ jbyte * const pBuf = s3jni_jbyteArray_bytes(jBa); - jsize nBa = (*env)->GetArrayLength(env, jBa); - if( nMax>=0 && nBa>(jsize)nMax ){ - nBa = (jsize)nMax; + jsize nBA = (*env)->GetArrayLength(env, jBa); + if( nMax>=0 && nBA>(jsize)nMax ){ + nBA = (jsize)nMax; /** From the sqlite docs: @@ -4181,15 +4219,15 @@ static void result_blob_text(int as64 /* true for text64/blob64() mode */, if( as64 ){ /* 64-bit... */ static const jsize nLimit64 = SQLITE_MAX_ALLOCATION_SIZE/*only _kinda_ arbitrary*/; - if( nBa > nLimit64 ){ + if( nBA > nLimit64 ){ sqlite3_result_error_toobig(pCx); }else if( asBlob ){ - sqlite3_result_blob64(pCx, pBuf, (sqlite3_uint64)nBa, + sqlite3_result_blob64(pCx, pBuf, (sqlite3_uint64)nBA, SQLITE_TRANSIENT); }else{ /* text64... */ if( encodingTypeIsValid(eTextRep) ){ sqlite3_result_text64(pCx, (const char *)pBuf, - (sqlite3_uint64)nBa, + (sqlite3_uint64)nBA, SQLITE_TRANSIENT, eTextRep); }else{ sqlite3_result_error_code(pCx, SQLITE_FORMAT); @@ -4197,27 +4235,27 @@ static void result_blob_text(int as64 /* true for text64/blob64() mode */, } }else{ /* 32-bit... */ static const jsize nLimit = SQLITE_MAX_ALLOCATION_SIZE; - if( nBa > nLimit ){ + if( nBA > nLimit ){ sqlite3_result_error_toobig(pCx); }else if( asBlob ){ - sqlite3_result_blob(pCx, pBuf, (int)nBa, + sqlite3_result_blob(pCx, pBuf, (int)nBA, SQLITE_TRANSIENT); }else{ switch( eTextRep ){ case SQLITE_UTF8: - sqlite3_result_text(pCx, (const char *)pBuf, (int)nBa, + sqlite3_result_text(pCx, (const char *)pBuf, (int)nBA, SQLITE_TRANSIENT); break; case SQLITE_UTF16: - sqlite3_result_text16(pCx, (const char *)pBuf, (int)nBa, + sqlite3_result_text16(pCx, (const char *)pBuf, (int)nBA, SQLITE_TRANSIENT); break; case SQLITE_UTF16LE: - sqlite3_result_text16le(pCx, (const char *)pBuf, (int)nBa, + sqlite3_result_text16le(pCx, (const char *)pBuf, (int)nBA, SQLITE_TRANSIENT); break; case SQLITE_UTF16BE: - sqlite3_result_text16be(pCx, (const char *)pBuf, (int)nBa, + sqlite3_result_text16be(pCx, (const char *)pBuf, (int)nBA, SQLITE_TRANSIENT); break; } diff --git a/ext/jni/src/org/sqlite/jni/capi/CApi.java b/ext/jni/src/org/sqlite/jni/capi/CApi.java index a721844744..302cdb760e 100644 --- a/ext/jni/src/org/sqlite/jni/capi/CApi.java +++ b/ext/jni/src/org/sqlite/jni/capi/CApi.java @@ -215,9 +215,10 @@ public final class CApi { ); /** - Results are undefined if data is not null and n<0 || n>=data.length. + If n is negative, SQLITE_MISUSE is returned. If n>data.length + then n is silently truncated to data.length. */ - public static int sqlite3_bind_blob( + static int sqlite3_bind_blob( @NotNull sqlite3_stmt stmt, int ndx, @Nullable byte[] data, int n ){ return sqlite3_bind_blob(stmt.getNativePointer(), ndx, data, n); @@ -325,11 +326,12 @@ public final class CApi { SQLITE_TRANSIENT for the final C API parameter. The byte array is assumed to be in UTF-8 encoding. -

Results are undefined if data is not null and - maxBytes>=utf8.length. If maxBytes is negative then results are - undefined if data is not null and does not contain a NUL byte. +

If data is not null and maxBytes>utf8.length then maxBytes is + silently truncated to utf8.length. If maxBytes is negative then + results are undefined if data is not null and does not contain a + NUL byte. */ - public static int sqlite3_bind_text( + static int sqlite3_bind_text( @NotNull sqlite3_stmt stmt, int ndx, @Nullable byte[] utf8, int maxBytes ){ return sqlite3_bind_text(stmt.getNativePointer(), ndx, utf8, maxBytes); @@ -354,7 +356,7 @@ public final class CApi { public static int sqlite3_bind_text( @NotNull sqlite3_stmt stmt, int ndx, @Nullable byte[] utf8 ){ - return (null == utf8) + return ( null==utf8 ) ? sqlite3_bind_null(stmt.getNativePointer(), ndx) : sqlite3_bind_text(stmt.getNativePointer(), ndx, utf8, utf8.length); } @@ -368,7 +370,7 @@ public final class CApi { signature but requires that its input be encoded in UTF-16 in platform byte order. */ - public static int sqlite3_bind_text16( + static int sqlite3_bind_text16( @NotNull sqlite3_stmt stmt, int ndx, @Nullable byte[] data, int maxBytes ){ return sqlite3_bind_text16(stmt.getNativePointer(), ndx, data, maxBytes); @@ -982,7 +984,7 @@ public final class CApi { necessary, however, and overloads are provided which gloss over that. -

Results are undefined if maxBytes>=sqlUtf8.length. +

Results are undefined if maxBytes>sqlUtf8.length.

This routine is private because its maxBytes value is not strictly necessary in the Java interface, as sqlUtf8.length tells diff --git a/manifest b/manifest index 293cd130bc..16ecd5eb71 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Add\smissing\sScalarFunction.java\sto\sJNI\sbuild. -D 2023-10-22T11:11:54.330 +C JNI:\simprove\sUB\sprotections\sin\ssqlite3_bind_blob/text/text16(). +D 2023-10-22T12:33:05.772 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -239,7 +239,7 @@ F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a3 F ext/jni/GNUmakefile 5c3ac326bf3853486ebe0d70819abc790cc65c412182ce4ebd5012b008d9b059 F ext/jni/README.md ef9ac115e97704ea995d743b4a8334e23c659e5534c3b64065a5405256d5f2f4 F ext/jni/jar-dist.make 030aaa4ae71dd86e4ec5e7c1e6cd86f9dfa47c4592c070d2e35157e42498e1fa -F ext/jni/src/c/sqlite3-jni.c 8d32ca0598a11370a9e92a6d111f38934c225056b42b13512175acf6e37eed4c +F ext/jni/src/c/sqlite3-jni.c 6f6df9657989e9ca2cfdcc2fe9a71c279de56d5c941adfd09a0f24256de35c8f F ext/jni/src/c/sqlite3-jni.h b4c413a0d0c734683da1049cfcf89e35ae2719759d0656ec0f8c57188f18cab8 F ext/jni/src/org/sqlite/jni/annotation/NotNull.java a99341e88154e70447596b1af6a27c586317df41a7e0f246fd41370cd7b723b2 F ext/jni/src/org/sqlite/jni/annotation/Nullable.java 0b1879852707f752512d4db9d7edd0d8db2f0c2612316ce1c832715e012ff6ba @@ -249,7 +249,7 @@ F ext/jni/src/org/sqlite/jni/capi/AggregateFunction.java bc29e986c866c2ddbbb9f93 F ext/jni/src/org/sqlite/jni/capi/AuthorizerCallback.java 7ed409d5449684616cc924534e22ff6b07d361f12ad904b69ecb10e0568a8013 F ext/jni/src/org/sqlite/jni/capi/AutoExtensionCallback.java 74cc4998a73d6563542ecb90804a3c4f4e828cb4bd69e61226d1a51f4646e759 F ext/jni/src/org/sqlite/jni/capi/BusyHandlerCallback.java 7b8e19810c42b0ad21a04b5d8c804b32ee5905d137148703f16a75b612c380ca -F ext/jni/src/org/sqlite/jni/capi/CApi.java 5d754b4bb57852d006ad046b2860eb23ba406f800846460b26beee5172df4fc3 +F ext/jni/src/org/sqlite/jni/capi/CApi.java bccb442ca81cd4decb1adae99006a60b7a9f54e5153842e738c01104e97d1de0 F ext/jni/src/org/sqlite/jni/capi/CallbackProxy.java 0bfd6e56e8265c2f05c9207665707285534d78f8466ef0e0430c65677f00943d F ext/jni/src/org/sqlite/jni/capi/CollationCallback.java e29bcfc540fdd343e2f5cca4d27235113f2886acb13380686756d5cabdfd065a F ext/jni/src/org/sqlite/jni/capi/CollationNeededCallback.java f81cf10b79c52f9b2e9247d523d29ae48863935f60420eae35f257c38c80ce95 @@ -2136,8 +2136,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P 4a0e2c6e17eefb296b7e91a13305b2260d6eb869a37bc5e5b79edaf3c19c810a -R 3fe1108f65f1caab77f9fbf8767ff3b7 +P b8258103fb433d9f5cfa15661b5edf4e60128fb4161d8a18e5cc3253e5aed72b +R 10dd3eddc3858c0c90f96d7af2dd213f U stephan -Z 479ae6fc35bc2a0cdd617f9441ccbd5d +Z 39a3acd541e66480809465cadb3002fa # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 571d755baa..bcfb4efa95 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -b8258103fb433d9f5cfa15661b5edf4e60128fb4161d8a18e5cc3253e5aed72b \ No newline at end of file +5c8383210a87d7f9d37a27053b5b1b6f41794fa8612826c68c1ca49c495cbd97 \ No newline at end of file