« first day (3837 days earlier)      last day (1094 days later) » 
00:00 - 16:0016:00 - 23:00

4:00 PM
Basically... it's really more of a "programmer defined string"
as in, it came somewhere from the PHP script
therefore is it safe
 
OK, so:
1) is_literal is a bad name, because it's not a literal. It's a "programmer vetted" string, of which literals are a subset.
2) That relies on the programmer being right that he vetted the string properly. Which... may not be the case. We *thought* the WHERE IN logic in the Drupal DBAL was safe, and would have marked it as such. We were wrong. Boom, Drupageddon, remote SQL injection.
3) So it's the same idea as taint variables, just inverted. All the same challenges apply, therefore.
 
I would like a better name for it, but can't think of one, and I think it was Derick who proposed it years ago (because they come from literal strings).
you don't mark anything as safe
for example?
 
Everything Sara has said about taint above. :-) (I wasn't involved in the taint discussion the last time it came up.)
 
I think it might be better to keep it genuinely as simple as literals and concatenation, no funkiness with implode() etc allowed
 
that's not implemented
 
4:03 PM
it's needed for the WHERE IN case.
 
library authors would have to provide their own escape hatches for more complex cases, and mark them appropriately
 
such as?
 
$queryBuilder->whereDangerousSql(...)
 
the only one I can find is when the SQL actually comes from the user, like phpMyAdmin
WHERE IN is defined above... field names can be done with a know-safe list of fields that can be used, e.g.
$order_fields = [
'name',
'created',
'admin',
];

$order_id = array_search(($_GET['sort'] ?? NULL), $order_fields);

$sql = ' ORDER BY ' . $order_fields[$order_id];
 
Views module in Drupal. :-) The table structure is user defined, the fields to select are user-defined, the sorting field is user-defined.
 
4:05 PM
@CraigFrancis I'm saying that the WHERE IN solution you give above shouldn't be included
 
And the user in this case is not a programmer.
 
@Crell see the previous example
 
allowing implode() dilutes the definition, and opens the gates to bugs
 
In Drupal you do not know the names of the possible fields at code time.
 
@IMSoP By including it, we get a full solution, where all SQL can be created by literals
 
4:05 PM
Because the schema is user-configured.
 
you don't have a safe list?
 
The direction this is going... sounds more like a solution in search of a problem.
 
It sounds more like a lack of problems?
 
@CraigFrancis leave that to the library; $queryBuilder->whereIn('id', [$foo, $bar, $baz])
id passes the simplest implementation of is_literal(), and the library can trivially generate the right placeholders for the rest
 
In Drupal's ORM (though they don't call it an ORM), the table name itself is user-defined. The field names in that table depend on the field type, which is one an infinite number of plugins. And there's nothing stopping those plugins from having user-defined field names.
 
4:07 PM
maybe, but the library itself could also ensure that the SQL it creates is safe... and query builders aren't always that easy to use.
 
A very simple "This string was taken directly from the source code" and let the library handle the rest.
 
^ this
 
I agree
 
anything more than that is drifting too far into taint-flag territory
 
I went as far as lsprintf as a cousin of concat/rope, but I don't think it needs to go further ...
 
4:09 PM
@JoeWatkins Out of curiosity, what think you to the idea of an object / class, so it can be type hinted, and fails on manipulations?
 
expensive
also, the domain of userland
 
Userland wouldn't be able to do it, as it would require a grammar change
 
It's just that previously people have said that the basic implementation did not support the WHERE IN case, so it's not complete, and they wouldn't vote for something that's incomplete.
 
we just want to make it possible for you to implement that kind of api ... trying to produce a string api is not going to happen ...
okay, but say we modify or introduce explode/implode/array* ... when exactly does that stop, what if someone says they want strtoupper to be literal aware ? the argument that we should introduce a new function for X is as strong as the argument that we should introduce a new function for Y ...
there's like a million functions ...
 
@CraigFrancis I can see that objection; but I think parametrised "where in" is just fiddly to do right anyway, so anyone writing it basically is writing a query builder
 
4:12 PM
@MarkR what ?
 
@Crell Depends on how you want to work it, this RFC is just phase 1, the building blocks... in your case, you might not be able to ensure the final SQL string is purely safe, but you could check that the field names are fine, and everything else is a literal... in my case, I don't do that, so I can guard the mysqli_prepare() function.
@JoeWatkins no, all the others introduce a risk... literal_implode and literal_array_fill take literals only, in their inputs and outputs... and make no changes to the values (as such)... anything else, even strtoupper causes modifications.
 
to be honest, I'd rather use a DBMS that supported an array (like Postgres) and a driver that didn't insist everything in and out be a string (unlike Postgres)
 
@JoeWatkins e.g. $query = _trusted("INSERT INTO ..."); where _trusted (or something) would only accept a single quoted string (no interpolation) and emit an opcode that produces a cached TrustedString that cannot be instantiated in any other way other than through that opcode (or trustedstrings members).
 
then query("... WHERE id = ANY(?)", [$myArray]) would work just fine
 
@IMSoP Having an array like structure for a parameter would be nice... but for now, a simple implode(',', array_fill(0, count($ids), '?')) gives you the variable number of parameters.
 
4:15 PM
@CraigFrancis "simple"
 
well, I do have a wrapper function ;-)
 
Has the benefits that it can't be used directly in any other string function without unboxing it, and can be type-hinted directly, and can provide its own limited set of trusted-aware functions, keeping the need to consider trusted flags entirely self contained.

Drawbacks, slight overhead and cannot be used in all the places where strings currently can be (array keys for example)
 
@CraigFrancis aha, a wrapper function ... AKA a query builder ... I rest my case
 
not a query builder, as I'm often having to write the SQL myself, as in, my code is working with SQL (performance thing).
so getting back a string of ", ?"* that is seen as a literal, and can be included in the SQL, and sent to the db, knowing it's still safe.
 
Although... with what's being proposed a userland library could have much the same effect by doing a literal check on its constructor args. I'm not super-keen on the idea of a string carrying hidden metadata, but yeah userland may be a better idea
 
4:23 PM
@CraigFrancis here's that danger-word "safe" again ... this is where we get into taint flag territory: I know that this particular string manipulation is "safe", so I'll just leave the flag unset
 
you don't get to toggle the flag
 
no, but the internals do
array_fill and implode do
 
@MarkR Yeah, but one day, maybe 8.3, I'd like functions like mysql_query to support a way to protect against unsafe use... or at least to start checking for and letting developers know that what they are doing is risky.
@IMSoP literal_array_fill and literal_implode would only work with the literal flags set on all inputs.
 
wait, so you'd actually add new functions?
 
2 new functions
 
4:25 PM
in that case, get rid of the ugly boilerplate while you're at it
repeat_literal(string $item, int $count, ?string $glue)
 
oh, yeah, sorry, wrote that a while ago... I was hoping it could pass the flag though If all inputs had the flag... but I think Dan suggested it would be easier to simply introduce some new functions.
 
or something
 
Flags to functions are generally discouraged.
 
flags in the sense of on the variables themselvs
the concern with modifying the existing functions is perf impact.
@Crell Just reading though the Drupageddon writeup, this would be protected as well, as the keys in the array wouldn't be allowed, they came from the user... you, the library author, using the is_literal check, would have needed to use a string defined within the script... so this would have stopped it from happening, or you would have worked around it (fine, but risky).
 
That presumes we can use a fixed name. Or rather, multiple fixed names with some persistent memory of which have been used if we want to allow multiple IN clauses in one query. (Which we did, because that's legal SQL.)
 
4:31 PM
I think I'd rather have a mysqli_literal_query.
 
Doesn't have to be a fixed name, that's why I use ? placeholders, and I can add as many of those as I like (ref the functions mentioned above).
 
But you can't mix ? and named placeholders in the same query in PDO, and Drupal made the decision early on to go all-named for usability.
 
well ...
 
@MarkR Maybe, but we are back to the issue that the main target for this is the vast number of programmers that are new, and have no idea what they are doing... we can't teach them all.
 
@CraigFrancis The problem is a) you break literally hundreds of millions of lines of code or b) people disable it all anyway and continue as is.
 
4:33 PM
there's never going to be a function that fixes stupid, if you know that big chunks of your query are coming directly from user input because that's the way you designed the database, and you don't have anything in place to mitigate the obvious flaws that introduces, then even if there were a function that fixed stupid, I'll assume it wouldn't get used ...
 
@Crell And for your case, you would have to work the same way as the HTML libraries... you take the responsibility of getting that bit right, have everything else checking (maybe you build up an array of SQL fragments that you implode together, each piece being checked for the literal flag, or doing the extra protection work).
 
So, no different than today, except if I am relying on a library that uses is_literal checks internally I am now screwed if I'm doing something that cannot be done with just literals.
 
If you change the default for something, the new default has to be something that (realistically) should have been in use anyway. You're talking about changing entire functionality
 
@MarkR Kinda, but if you're already using a library like Doctrine, that could probably switch that check on fairly easily (as most methods area already safe, it's just things like the where() method)... start with a switch to enable notices, while you fix the mistakes, then go into enforcement mode.
@Crell Such as?
@MarkR I don't think the functionality, the way it works, changes... unless you have been using escaping (ref SQL), in which case that's being deprecated at first, then, in a few years, starts being blocked by default (with a flag to switch off, but one which is clearly saying you are doing it wrong, and becomes the teaching moment that we don't currently have).
 
We've been over the such as.
 
4:42 PM
Ideally, in ~10 years time, any system that has an injection vulnerability, you would start asking where the mistake was, and this flag clearly defines the 2 types of values (commands from the programmer; vs anything else, typically user supplied values)... the everyday programmer cannot get that wrong (it's done by PHP), and the vulnerability can now only be in the library or further up the chain (written by someone who knows the issues to address).
@Crell Well, you've been talking about your library, as a library author... you're now talking about the normal every-day programming.
 
Library authors are normal every day programmers. :-)
 
Yes, but you're held to a much higher standard, and should have more eyes on the code, and can have issues fixed in one place (and assuming people apply the patches), get those issues fixed quickly
Doctrine ORM has 91,235,595 installs (in theory), assuming Doctrine checked for non-literal values (maybe a notice, maybe an exception, you're choice), none of those projects would be able to write an SQLi vulnerability into their code by mistake.
 
1) You have not proven that it's impossible, just likely harder to do.
2) It means none of those projects would be able to write SQL more complex than what can be done with strict literals, even if their business logic requires it.
 
1) I can't find a single way in which it could be done, where parameterised queries is sold by the premise that injection is impossible (assuming, as this RFC could be used to enforce, that user defined values are not in the SQL string).
2) The reason I would like the literal_array_fill and literal_implode methods is to allow that to happen... those are the keys to allowing the business logic to contain that WHERE IN undefined number of parameters...
whereas the field names, that could use the ORDER BY example I gave earlier (where the field names come from the list of allowed literals), or you have the library handle that exception, where it does the extra checks around that (with the rest of that query, and the vast majority of all other queries, being protected by this check).
 
I've done some rewrites on the RFC; feel free to undo anything you disagree with, but I've tried to tone down the "this will fix everything", and make clearer where it would be useful
 
4:57 PM
@NikiC Adding that results in this from any test creating a backtrace within a fiber. Looks like it's using the 64-bit version of that library.
Assuming the stack addresses being on the heap is causing that.
Or rather in mmap'd memory.
 
thinking about the WHERE IN case, if you've got a helper function for it anyway, you can do it with just concatenation: $string = '?'; for ($i=0; $i<count($args); $i++) { $string .= ',?'; }
I may have a fence-post error, there; I hate C-style for loops
 
@IMSoP oh, ffs, yes, thank you, that would do it... why, after all these years didn't I see that... thanks.
 
heh; implode certainly feels like the logical thing to use
although, my first thought was actually str_repeat
then trim off the extra comma
 
ok, yes, that's that bit gone... and yes, str_repeat kina works, but yo need to trim
*you got it
 
which is kind of the problem with white listing anything for is_literal: you wanted array_fill and implode because it was what your implementation used, but someone else would want str_repeat, or something else we haven't thought of
 
5:04 PM
yep, I was trying to avoid all functions, but for some reason I didn't see a way around that one (to be fair, I've asked a few other people, but maybe they weren't as invested, and trying to ignore the lunatic in the corner who thinks they can solve the injection problem).
 
something I didn't get to on the RFC is that the examples don't show the values being passed to or returned from functions at all, which is a rather important part of the feature
the distinction between passing the value and manipulating it is a subtle one which people might miss
 
I did have a very long RFC before, with lots of examples, that got split and most of the examples are on the justification page.
(and even then I think I cut a lot of them).
 
yeah, conciseness is hard
function example(string $input): string {
    if ( ! is_literal($input) ) {
        throw new Exception('Non-literal detected!');
    }
    return $input;
}
example('hello'); // OK
example(example('hello')); // still OK: the return value is unchanged, so still marked as a literal
example(strtoupper('hello')); // Exception thrown: the result of strtoupper is a new, non-literal string
 
@IMSoP Thanks for the updates to the RFC, that looks much better
yep, perfect
not sure I'd do example(example()), but maybe there is a use case for that.
 
@CraigFrancis Anyone selling parameterized queries as an absolute fix to SQL injection rather than "eliminates most SQL injection" is selling snake oil and should be treated as such.
 
5:12 PM
With the "Proposed Voting Choices", I was thinking of having 2 questions... 1 being a basic is_literal() function that does not support string concat, where developers would need to use a library (query builder) or special functions like literal_implode(), not ideal, but avoids the performance concerns... 2, the extended version that does support concat... just so I can see the breakdown of those who just don't want this feature at all, and those who are concerned about the perf impact.
 
yeah, maybe a second sample function would be clearer
 
@Crell Really?
 
the point was to show that as well as passing into a function, a literal can pass out of one
or both at once
 
or maybe $a = 'a'; $b = 'b'; example($a . $b); ?
oh
wait a minute
the concat isn't necessarily allowed yet
either way, the basic example is a good idea.
 
what concat isn't allowed?
 
5:15 PM
sorry
yes, ideally
but I worry that the whole thing might be chucked out if people just worry about perf issues... I really hope it's not, I'd really want it, and think it would be very useful (why I've been asking for it in the proposed implementation, ready for testing), but if it's going to be seen as a major blocker, as in nothing is accepted, I'd still rather have the basic non-concat version... hope I'm explaining that correctly.
 
@JoeWatkins Any chance you know what is going wrong here?
Or rather how to get the compiler to use the correct lib?
This is a little outside my pay grade, I've never had to deal with compiling 32-bit on 64 :)
 
is the correct version of the library present on the machine ?
@CraigFrancis functions like literal_implode belong in userland
function literal_implode(string $glue, array $pieces) : string {
    if (!is_literal($glue)) {
        throw new ValueError("glue must be a literal string");
    }

    $result = null;

    foreach ($pieces as $piece) {
        if (!$result) {
            $result = $piece;
        } else {
            $result = lsprintf(
                "%s%s%s", $result, $glue, $piece);
        }
    }

    return $result;
}

$implode = literal_implode(", ", ["hello", "world", 42]);
giving people too many choices is not a good idea ... imo try to get the complete implementation through ... if you remove the vm stuff you're left with something questionably useful and inconsistent ...
 
@JoeWatkins Yep, you're right, and so is Rowan... I've been staring at that bit of the problem for years (not joking), and I don't know why I didn't think about using a loop and normal concat.
 
@Crell It's not really about lookahead in this case. Basically it's a shift/reduce conflict. Usually you can use precedence to tell Bison how to behave, which case is desired. gnu.org/software/bison/manual/html_node/Precedence.html But I'm not sure if that works in this case.
 
I'd really like to keep it in.
 
5:27 PM
concat will not produce a literal string if you use non-strings ...
 
yep, good point... so your lsprintf() helps there
 
@IluTov Hrm. So what do you suggest?
 
@Crell I don't know. Maybe there's a way to work around it, I'm definitely not a Bison expert. If not we have two options: 1. Require parens for complex cases like you suggest 2. Use a different syntax altogether that avoids the problem.
 
@JoeWatkins I would assume so… it's part of gcc.
 
Hm. Sounds like we should at least ponder if there are alternate syntaxes that would avoid the issue, although I'm suspecting not. Any alternate would either run into the same problem, or be so different as to be non-obvious what it's a pattern for.
 
5:34 PM
I assume some flag is set incorrectly though.
 
I got nothin
 
Well, you didn't set -msse2 when you compiled, so let's try removing that.
Seems logical, since that needs to be defined to compile that path.
 
yeah
 
5:59 PM
@JoeWatkins, thanks for your notes earlier, I don't think the 1 char optimisation would cause an issue (if anyone can find a way of making an exploit out of that, I'd be amazed; and it still teaches the basic principle).
Functions, looks like they aren't needed (woohoo)... unless we do end up needing a [literal_combine from Dan](https://github.com/php/php-src/compare/master...Danack:is_literal_attempt_two#diff-2b0486443df74cd919c949f33f895eacf97c34b8490e7554e032e770ab11e4d8R2761) as an alternative to string concat (but I hope you're right, and we don't need that).
 
6:50 PM
@Trowski I don't think the used include matters here
It's most likely related to the fact that we test with -msse2
 
@NikiC Yep, removing -msse2 seems to have worked.
Build isn't done, but it's passed the fiber portion.
 
It's not obvious to me what the issue is though
 
Me either, I don't understand why it's using the x86_64 header file.
I assume if it used the proper i386 file all would be fine.
 
I doubt it
It may be related to stack alignment
 
Could be, I can manually align the stack. It's done in the make_fcontext assembly, but maybe not for i386, which then matters when executing on x86_64.
 
6:59 PM
Needs some local debugging ^^
Bit hard to guess from just a stack trace
 
No, looks like the i386 assembly aligns the stack pointer too.
 
It might be worthwhile to rerun that with --asan passed to the test runner
Otherwise all the zmm parts aren't sanitized
So root cause may be hidden
 
@NikiC Uh, where would I add that?
 
@Trowski To this line: github.com/php/php-src/blob/…
 
Oh, so just add runTestsParameters: '--asan' to parameters.
Either way I guess, same result :P
 
7:45 PM
Incident with GitHub Actions ・ GitHub Pages has Partial Outage
All issues have been resolved!
 
8:22 PM
@NikiC I wonder if functions using SSE2 need __attribute__ ((force_align_arg_pointer))?
 
9:04 PM
@NikiC Adding the -mstackrealign flag fixes it, so it seems that's the issue.
There's only a handful of functions that use SSE2, so adding that flag would be trivial.
Which, from what I understand, adds a single instruction to align the stack pointer before the function, so not a big penalty. Maybe this has always been necessary but it's only now being revealed?
 
9:33 PM
> Realign the stack at entry. On the x86, the -mstackrealign option generates an alternate prologue and epilogue that realigns the run-time stack if necessary. This supports mixing legacy codes that keep 4-byte stack alignment with modern codes that keep 16-byte stack alignment for SSE compatibility. See also the attribute force_align_arg_pointer, applicable to individual functions.
That sounds like it should be necessary only if linking "legacy codes"
 
@NikiC I'm not really sure what they mean there. I'm wondering if compiling it as 32-bit is causing 4-byte stack alignment (so like it's legacy code) and the function called there then requires 16-byte alignment.
So it's not "legacy code," but being compiled like legacy code and linking to x86_64 code?
 
@Trowski sysv abi requires 16 byte stack alignment even for 32-bit
 
9:50 PM
@NikiC The stack is initially 16 byte aligned. Not sure how, but it appears that goes wrong somewhere in the call stack.
 
 
1 hour later…
10:56 PM
@FélixGagnon-Grenier reddit.com/rpan/r/TheYouShow/mtmh28 We did a live show today
 
00:00 - 16:0016:00 - 23:00

« first day (3837 days earlier)      last day (1094 days later) »