165790 2016-12-12 22:52:13 -0800 makeString UChar* adaptor is silly 2022-02-28 03:46:39 -0800 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 190187 P2 Normal --- 147142 1 jfbastien jfbastien achristensen andersca ap beidson benjamin cdumez cmarcelo commit-queue darin dbates fpizlo jfbastien mark.lam mcatanzaro sam oldest_to_newest 1259344 0 jfbastien 2016-12-12 22:52:13 -0800 This code seems silly: unsigned length = 0; while (m_characters[length]) ++length; if (length > std::numeric_limits<unsigned>::max()) CRASH(); I'll clean it up as a follow-up to #147142. I'd appreciate guidance on what a good upper-bound is. IMO getting to anything "big" without finding a 0 means we're severely broken already, so crashing is the right action. I'm just not sure: 1. Has this ever happened? Even ignoring the unsigned comparison to its maximum, we'd need to have 4GiB of continuous non-zero readable data. 2. What would a better limit be for strings? I'm not a fan of hard-coded values, but there's got to be a number which ought to be Good Enough for Everyone™. 1259769 1 darin 2016-12-14 09:27:36 -0800 String length overflow checks have indeed been critical for security in the past. We started without having almost any of them in WebKit and added lots of them over the years as we resolved specific security issues, and especially as we started dealing with a mix of 32-bit and 64-bit values. Some checks that seem obviously impossible to hit actually can be hit quite easily on 64-bit systems with lots of RAM, when code runs that intentionally tries to get the engine to generate a giant string. It’s not immediately obvious from looking at one function what callers might be directly or indirectly controlled by an attacker looking for a vulnerability. Adding a new lower limit is an interesting project; it’s hard to know what cases might legitimately lead to a long string just from looking at the lowest level. You typically have to look at all the callers to understand what they are doing. Having a lower limit simply changes a "hang" into a "crash", so the benefit of finding a lower value is a modest one, unless we use the limit is some other way, perhaps throwing a JavaScript exception or something like that. Typically when we add the overflow checks, most don’t try to implement a global “sensible policy”, but rather catch things that actually reach points we know the implementation can’t handle without overflowing or such. It’s much harder to come up with an arbitrarily “biggest normal value” limit. Not that it can’t be done, but it’s more challenging. Strings stored in WTF::StringImpl have a maximum length smaller than the maximum 32-bit unsigned integer, depending on how the StringImpl is created. The practical limit is in the neighborhood of 2^31. 1259771 2 darin 2016-12-14 09:29:16 -0800 (In reply to comment #0) > This code seems silly: > > unsigned length = 0; > while (m_characters[length]) > ++length; > > if (length > std::numeric_limits<unsigned>::max()) > CRASH(); Wait, that’s beyond silly. That’s just dead unreachable code. If the length is greater than the maximum unsigned, then we will have already overflowed before checking it. I’m surprised the compiler doesn’t issue an error or warning. The length local variable had a 64-bit type then the code would be “merely silly”. 1259773 3 darin 2016-12-14 09:29:34 -0800 (In reply to comment #2) > The length local variable had a 64-bit type then the code would be “merely > silly”. If the ... 1259774 4 jfbastien 2016-12-14 09:30:22 -0800 (In reply to comment #2) > (In reply to comment #0) > > This code seems silly: > > > > unsigned length = 0; > > while (m_characters[length]) > > ++length; > > > > if (length > std::numeric_limits<unsigned>::max()) > > CRASH(); > > Wait, that’s beyond silly. That’s just dead unreachable code. If the length > is greater than the maximum unsigned, then we will have already overflowed > before checking it. I’m surprised the compiler doesn’t issue an error or > warning. > > The length local variable had a 64-bit type then the code would be “merely > silly”. :-) 1259781 5 darin 2016-12-14 09:39:17 -0800 I suggest we change this to use size_t and otherwise leave it alone, based on the rationale I gave above. But perhaps there’s something even better we can do. Seems unimportant to optimize this case; null-character terminated UChar strings are super rare on iOS and macOS, and come up mostly in when dealing with results of functions on Windows, so this isn’t likely to be used in hot code. 1260177 6 297201 jfbastien 2016-12-15 10:40:52 -0800 Created attachment 297201 patch I don't think size_t is a good idea: on 32-bit platforms we're back to the same problem. On 64-bit platforms we'll just keep counting as we were before: way too much! Better to just call it a day IMO. If we ever encounter a 1GiB string without a null terminator in it then we're already having a bad time. WDYT? 1260217 7 darin 2016-12-15 12:45:03 -0800 (In reply to comment #6) > on 32-bit platforms we're back to the same problem On 32-bit platforms there's no way to overflow a 32-bit usigned integer walking through a string a byte at a time unless the entire computer has no zero bytes in it. So the code is dead code, and code that does no harm. The whole point of the code, I think, is to prevent us from overflowing 32 bits and passing an inappropriately small value to the create function. > On 64-bit platforms we'll just keep counting as we were before: way too > much! Better to just call it a day IMO. I don’t know what “better to call it a day” means in practice. > If we ever encounter a 1GiB string without a null terminator in it then > we're already having a bad time. WDYT? I don’t think we should waste our time choosing an arbitrary limit. I don’t think a 1GiB limit is better than a 4GiB limit. Maybe you could give a concrete example where you think the smaller limit will make things significantly better? I think maybe we should could just remove the check entirely, because the symptom of creating a too-small string might be harmless. 1260964 8 ap 2016-12-17 14:49:50 -0800 One scenario where we've seen large blobs of data is dealing with files (especially uploading videos to YouTube). Indeed, it seems unlikely that those will ever be passed through this adapter. 1261317 9 297504 jfbastien 2016-12-19 20:41:56 -0800 Created attachment 297504 patch You're right, the limit isn't helpful and deleting the check is better. I bumped the length to a size_t though, so at least it doesn't wrap around if we do get a huge string (though as was pointed out the subsequent string may have its own arbitrary limit). 1261680 10 297504 darin 2016-12-20 12:45:42 -0800 Comment on attachment 297504 patch View in context: https://bugs.webkit.org/attachment.cgi?id=297504&action=review > Source/WTF/wtf/text/StringConcatenate.h:147 > + size_t length() const { return m_length; } Where are the callers to this length function? Did we audit them to make sure they correctly handle size_t rather than unsigned? 1262117 11 297504 darin 2016-12-21 13:24:22 -0800 Comment on attachment 297504 patch View in context: https://bugs.webkit.org/attachment.cgi?id=297504&action=review >> Source/WTF/wtf/text/StringConcatenate.h:147 >> + size_t length() const { return m_length; } > > Where are the callers to this length function? Did we audit them to make sure they correctly handle size_t rather than unsigned? I will likely immediately r+ this patch once I get the answer to this, either by researching it myself, or by you answering here. 1262133 12 jfbastien 2016-12-21 13:33:52 -0800 (In reply to comment #11) > Comment on attachment 297504 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297504&action=review > > >> Source/WTF/wtf/text/StringConcatenate.h:147 > >> + size_t length() const { return m_length; } > > > > Where are the callers to this length function? Did we audit them to make sure they correctly handle size_t rather than unsigned? > > I will likely immediately r+ this patch once I get the answer to this, > either by researching it myself, or by you answering here. Sorry for the slow response rater on this. It's a great question and I want to go through the code properly before getting back to you. I'll do this soon, but have a few other things I want to get to first. 1392608 13 jfbastien 2018-01-24 09:55:41 -0800 Apparently this will be flagged as a tautological compare by upcoming clang: length > std::numeric_limits<unsigned>::max() Should we just remove it for now (since it is tautological) or use size_t as I suggest? I don't have time to audit code as I said I would (then forgot...). 1399143 14 297504 beidson 2018-02-14 10:36:37 -0800 Comment on attachment 297504 patch Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form. If this patch is still important please rebase it and post it for review again. 1402274 15 fpizlo 2018-02-26 21:23:24 -0800 (In reply to JF Bastien from comment #13) > Apparently this will be flagged as a tautological compare by upcoming clang: > length > std::numeric_limits<unsigned>::max() > > Should we just remove it for now (since it is tautological) or use size_t as > I suggest? I don't have time to audit code as I said I would (then > forgot...). I think that you should remove the comparison first and then do the size_t thing and audit. 1465733 16 mark.lam 2018-10-02 13:06:42 -0700 This issue will be handled when we fix https://bugs.webkit.org/show_bug.cgi?id=190187. *** This bug has been marked as a duplicate of bug 190187 *** 297201 2016-12-15 10:40:52 -0800 2016-12-19 20:41:56 -0800 patch 0001-makeString-UChar.patch text/plain 1952 jfbastien RnJvbSA1NTdmMmY3YThkODNkODkyYTg4MWFmYTFkMTBhZDUyMmJmNDdhOGUzIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKRiBCYXN0aWVuIDxqZmJhc3RpZW5AYXBwbGUuY29tPgpEYXRl OiBUaHUsIDE1IERlYyAyMDE2IDEwOjM1OjQ1IC0wODAwClN1YmplY3Q6IFtQQVRDSF0gbWFrZVN0 cmluZyBVQ2hhcgoKLS0tCiBTb3VyY2UvV1RGL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAgICAg fCAxNiArKysrKysrKysrKysrKysrCiBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0NvbmNhdGVu YXRlLmggfCAgNSArKystLQogMiBmaWxlcyBjaGFuZ2VkLCAxOSBpbnNlcnRpb25zKCspLCAyIGRl bGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nIGIvU291cmNlL1dU Ri9DaGFuZ2VMb2cKaW5kZXggYzNmMjdkYS4uNDU2ZWVkOSAxMDA2NDQKLS0tIGEvU291cmNlL1dU Ri9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAor MjAxNi0xMi0xNSAgSkYgQmFzdGllbiAgPGpmYmFzdGllbkBhcHBsZS5jb20+CisKKyAgICAgICAg bWFrZVN0cmluZyBVQ2hhciogYWRhcHRvciBwZXJmb3JtcyB1c2VsZXNzIHJhbmdlcyBjaGVjawor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY1NzkwCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIG9sZCBj aGVjayBjb3VsZCBuZXZlciBmaXJlLCBpdCBub3cgZW5mb3JjZXMgYW4gYXJiaXRyYXJ5IGJ1dAor ICAgICAgICBsYXJnZSBsaW1pdC4gV2VyZSB3ZSBldmVyIHRvIGhpdCBhIGxhcmdlIGxpbWl0IChh bnkgbGltaXQpIGl0CisgICAgICAgIHdvdWxkIG1lYW4gdGhhdCB3ZSd2ZSByZWNlaXZlZCBhIGh1 Z2Ugc3RyaW5nLCBhbmQgd2UndmUgdHJhdmVyc2VkCisgICAgICAgIGFsbCBvZiBpdHMgbWVtb3J5 LCBhbmQgaGF2ZW4ndCBmb3VuZCBhIHNpbmdsZSB6ZXJvLiBUaGF0J3MgaGlnaGx5CisgICAgICAg IHVubGlrZWx5LCBhbmQgaWYgZXZlciBpdCBoYXBwZW5zIHRoZW4gc29tZXRoaW5nIHJlYWxseSBi YWQgaXMKKyAgICAgICAgZ29pbmcgb24uCisKKyAgICAgICAgKiB3dGYvdGV4dC9TdHJpbmdDb25j YXRlbmF0ZS5oOgorCiAyMDE2LTEyLTE1ICBZdXN1a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21h aWwuY29tPgogCiAgICAgICAgIFtKU0NdIE9wdGltaXplIEtyYWtlbiBzdHJpbmdpZnkKZGlmZiAt LWdpdCBhL1NvdXJjZS9XVEYvd3RmL3RleHQvU3RyaW5nQ29uY2F0ZW5hdGUuaCBiL1NvdXJjZS9X VEYvd3RmL3RleHQvU3RyaW5nQ29uY2F0ZW5hdGUuaAppbmRleCBhZmZiN2UxLi4yYzMzMTYxIDEw MDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0NvbmNhdGVuYXRlLmgKKysrIGIv U291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdDb25jYXRlbmF0ZS5oCkBAIC0xMzgsMTAgKzEzOCwx MSBAQCBwdWJsaWM6CiAgICAgICAgIDogbV9jaGFyYWN0ZXJzKGNoYXJhY3RlcnMpCiAgICAgewog ICAgICAgICB1bnNpZ25lZCBsZW5ndGggPSAwOwotICAgICAgICB3aGlsZSAobV9jaGFyYWN0ZXJz W2xlbmd0aF0pCisgICAgICAgIGNvbnN0IHVuc2lnbmVkIGxpbWl0ID0gMSA8PCAzMDsKKyAgICAg ICAgd2hpbGUgKG1fY2hhcmFjdGVyc1tsZW5ndGhdICYmIGxlbmd0aCA8IGxpbWl0KQogICAgICAg ICAgICAgKytsZW5ndGg7CiAKLSAgICAgICAgaWYgKGxlbmd0aCA+IHN0ZDo6bnVtZXJpY19saW1p dHM8dW5zaWduZWQ+OjptYXgoKSkgLy8gRklYTUUgdGhpcyBpcyBzaWxseSBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY1NzkwCisgICAgICAgIGlmIChsZW5ndGggPj0g bGltaXQpCiAgICAgICAgICAgICBDUkFTSCgpOwogCiAgICAgICAgIG1fbGVuZ3RoID0gbGVuZ3Ro OwotLSAKMi4xMC4xCgo= 297504 2016-12-19 20:41:56 -0800 2022-02-28 03:46:39 -0800 patch 0001-makeString-max.patch text/plain 2377 jfbastien RnJvbSA4MTg1ODgyNjBmNmZkNzcxYjI3MGIwZmRmODZjOTY2ODA1YWZiZjAwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKRiBCYXN0aWVuIDxqZmJhc3RpZW5AYXBwbGUuY29tPgpEYXRl OiBNb24sIDE5IERlYyAyMDE2IDIwOjM4OjIzIC0wODAwClN1YmplY3Q6IFtQQVRDSF0gbWFrZVN0 cmluZy1tYXgKCi0tLQogU291cmNlL1dURi9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAgIHwg MTUgKysrKysrKysrKysrKysrCiBTb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0NvbmNhdGVuYXRl LmggfCAgOSArKystLS0tLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTggaW5zZXJ0aW9ucygrKSwgNiBk ZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5nZUxvZyBiL1NvdXJjZS9X VEYvQ2hhbmdlTG9nCmluZGV4IDZlYmRhM2MuLmI0ZjBhNjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X VEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAK KzIwMTYtMTItMTkgIEpGIEJhc3RpZW4gIDxqZmJhc3RpZW5AYXBwbGUuY29tPgorCisgICAgICAg IG1ha2VTdHJpbmcgVUNoYXIqIGFkYXB0b3IgaGFzIHVzZWxlc3MgY2hlY2sKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2NTc5MAorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL3RleHQvU3RyaW5nQ29u Y2F0ZW5hdGUuaDogVGhlIGNoZWNrIGNvdWxkIG5ldmVyIGZpcmUgYmVjYXVzZQorICAgICAgICBh biBpbnRlZ2VyIGNhbiBuZXZlciBiZSBncmVhdGVyIHRoYW4gaXRzIHR5cGUncyBtYXhpbXVtLiBJ bnN0ZWFkCisgICAgICAgIG9mIGVuZm9yY2luZyBhbiBhcmJpdHJhcnkgbGltaXQsIGp1c3QgcmVt b3ZlIHRoZSBjaGVjaywgYnV0IGNvdWxkCisgICAgICAgIHVzaW5nIHNpemVfdCB0byBtYWtlIHN1 cmUgYW55IHJpZGljdWxvdXMgc3RyaW5nIGtlZXBzIHdvcmtpbmcuIFRvCisgICAgICAgIGhpdCBz dWNoIGEgc3RyaW5nIG9uZSB3b3VsZCBuZWVkIGEgNjQtYml0IGFyY2hpdGVjdHVyZSB3aXRoIG92 ZXIKKyAgICAgICAgNEdpQiBvZiBub24temVybyBkYXRhLiBJbiBzdWNoIGEgY2FzZSwgd2UnbGwg YmUgY29ycmVjdCwgYnV0IHRoZQorICAgICAgICBzdHJpbmcgd2UncmUgdHJ5aW5nIHRvIGNvbmNh dGVuYXRlIGludG8gbWF5IGdldCBzYWQuCisKIDIwMTYtMTItMTkgIE1hcmsgTGFtICA8bWFyay5s YW1AYXBwbGUuY29tPgogCiAgICAgICAgIFJvbGxpbmcgb3V0IHIyMDk5NzQgYW5kIHIyMDk5NTIu IFRoZXkgYnJlYWsgc29tZSB3ZWJzaXRlcyBpbiBteXN0ZXJpb3VzIHdheXMuIFN0ZXAgMjogUm9s bG91dCByMjA5OTUyLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdDb25j YXRlbmF0ZS5oIGIvU291cmNlL1dURi93dGYvdGV4dC9TdHJpbmdDb25jYXRlbmF0ZS5oCmluZGV4 IGFmZmI3ZTEuLjUzZTA0NDQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL3RleHQvU3RyaW5n Q29uY2F0ZW5hdGUuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi90ZXh0L1N0cmluZ0NvbmNhdGVuYXRl LmgKQEAgLTEzNywxNyArMTM3LDE0IEBAIHB1YmxpYzoKICAgICBTdHJpbmdUeXBlQWRhcHRlcihj b25zdCBVQ2hhciogY2hhcmFjdGVycykKICAgICAgICAgOiBtX2NoYXJhY3RlcnMoY2hhcmFjdGVy cykKICAgICB7Ci0gICAgICAgIHVuc2lnbmVkIGxlbmd0aCA9IDA7CisgICAgICAgIHNpemVfdCBs ZW5ndGggPSAwOwogICAgICAgICB3aGlsZSAobV9jaGFyYWN0ZXJzW2xlbmd0aF0pCiAgICAgICAg ICAgICArK2xlbmd0aDsKIAotICAgICAgICBpZiAobGVuZ3RoID4gc3RkOjpudW1lcmljX2xpbWl0 czx1bnNpZ25lZD46Om1heCgpKSAvLyBGSVhNRSB0aGlzIGlzIHNpbGx5IGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjU3OTAKLSAgICAgICAgICAgIENSQVNIKCk7Ci0K ICAgICAgICAgbV9sZW5ndGggPSBsZW5ndGg7CiAgICAgfQogCi0gICAgdW5zaWduZWQgbGVuZ3Ro KCkgY29uc3QgeyByZXR1cm4gbV9sZW5ndGg7IH0KKyAgICBzaXplX3QgbGVuZ3RoKCkgY29uc3Qg eyByZXR1cm4gbV9sZW5ndGg7IH0KICAgICBib29sIGlzOEJpdCgpIGNvbnN0IHsgcmV0dXJuIGZh bHNlOyB9CiAKICAgICBOT19SRVRVUk5fRFVFX1RPX0NSQVNIIHZvaWQgd3JpdGVUbyhMQ2hhciop IGNvbnN0CkBAIC0xNjQsNyArMTYxLDcgQEAgcHVibGljOgogCiBwcml2YXRlOgogICAgIGNvbnN0 IFVDaGFyKiBtX2NoYXJhY3RlcnM7Ci0gICAgdW5zaWduZWQgbV9sZW5ndGg7CisgICAgc2l6ZV90 IG1fbGVuZ3RoOwogfTsKIAogdGVtcGxhdGU8PgotLSAKMi45LjMKCg==