I got a few small changes to the ticket system, which I grouped into the branch tkt-improvements.

The changes contain:

• I got annoyed changing things in /tktsetup for testing purposes again and again and having to click through to it. To improve I added a submenu to the sub-pages of /tkt_setup that goes back to it, and also made 'Cancel' consistently go to the parent page. (fossil:f5619ef3e0bfa164)
• The old ticket fossil:3bcacd36ff9e8820 asked for showing the discussion so far on the /tktedit page, which I added. (fossil:ff009a8f0e482a86)
• In ticket reports, if you enable multiple rows per ticket (which can be done by naming field with leading underscore), then an empty row is added for each ticket, which is intended as a separator (as indicated by code comment). I changed it to contain a <hr>, so that it is visibly clearer to understand as separator. (fossil:7b08586a41d11f19)
• Ticket fossil:04ba0f70b816cebe asks for linking the version field in /tktview to the respective version. I added TH1-code to the default template, that checks if the content is either a valid tagname or a valid checkin-hash, and then links to it accordingly. (fossil:44bd394f0a618264)
  • EDIT: With the fixes for the injection issue that code now also checks the version-string before even attempting to query the DB. For checkin-hashes that looks for values containing only numbers and a-f, for tags alphanumeric values and underscores (_), dots (.) and hyphen (-). Values in the field not matching these will not get a link.
• Ticket fossil:14bc0ad52d919860 asks to show creation time as well. I am not aware if back at the time the ticket was created the tkt_ctime field already existed, but it does now. So I added it to the default template for new ticket reports and made the column names more clear (fossil:86c1910ffbd1b642), added the creation time to /tktview (fossil:814a417b4d2d1071) and also added human readable ages for both modification time and creation time to /tktview (fossil:8b48a054edda1a17).
• The submenu to delete a ticket report in /rptedit wasn't working. That is because of the CSRF-check (which is a good thing). I removed the submenu to avoid confusion. The functionality to delete a report is still available if you click the button in the form. (fossil:7c91416166a7ecbb)

As these are different things covered, it might be prudent to remove some of these before merging if you object to a specific thing in the list. I just bundled them in one branch, as each seems to small to justify it's own branch.

All of these changes are on the small side, but I think they all add a little quality of life to the ticket system to justify their inclusion.

I tried to implement as much as possible in changeable templates. This means that a lot of these things (like creation date or linking the version field) are customizable for the repository admins. It also means a lot will not show up in repositories until templates are reverted to default.

What do you think, are these changes fit for inclusion in trunk?

If there are no objection I would go on and merge this.

Please adjust source code files for a maximum line length of 80 characters.

I think I fixed all occurences of longer lines I introduced.

SQL Injection - DO NOT MERGE

SQL injection on the $foundin variable. For example, enter a new ticket where the first character of the "Version:" entry is single-quote (U+0027).

Oh, damn. I'll look into it or if I cannot fix it drop that change (which links the foundin-field to appropriate checkins). But let's see if I can catch it first. Thanks for the hint.

Thanks for the fix of the SQL injection. I added quoting for HTML and URL.

In my test repository I created some very wild tags to test this and found no problem after both our changes.

Also I put up a guard that I only call the query if the string matches a whitelist of characters. I wasted a bit of time here with string match, which isn't sufficient. But regexp does work. This guard means my wild tags aren't working anymore, as the code doesn't even look for such a tag, but I think that is fine as few will put in such characters in their tags and this only means that no link will be generated. Also this is only the default TH1 for the ticket-view, admins can change it at their will to allow for more stuff.

The only thing that bugs me, is that I could not properly quote the minus ('-') for the regexp in TH1, so that this isn't allowed in tags for links to be created as well (I allowed for numbers, letters, dot '.' and underscore '_', which I assume may turn up in tags representing versions).

EDIT: Got the minus added to the pattern, but I only managed via unicode escape.

The only thing that bugs me, is that I could not properly quote the minus ('-') for the regexp in TH1, so that this isn't allowed in tags for links to be created as well (I allowed for numbers, letters, dot '.' and underscore '_', which I assume may turn up in tags representing versions).

In regexes, a hyphen can be matched either by having it outside of a [...] block or place it at the start of [-...], where [-] by itself should match a single hyphen.

We use lots of hyphens in tag names, and some repos (like Tcl Core's) exclusively uses hyphens as separators¹.

1. ^{^} Presumably, though a handful of Tcl core dev were unable to confirm this for me, because of compatibility with filesystem names on some old systems, like an inability to handle multiple dots in a name.

Thanks. I managed with Unicode-escape, but this is the better solution as the unicode escape hides what is happening (except you memorized character codes, which I don't).

Having a look at that code, I was confused by the \ before [ ] $. Which are probably part of the TH1-escaping and not the regexp itself. Maybe having the pattern in { } is more readable - certainly less escape-y - but someone with profound TH1/TCL knowledge should judge:

set tagpattern {^[-0-9A-Za-z_\\.]+$}

(already contains the hyphen upfront, btw.)

Thanks alot!

I never in my life used TCL, so coming to fossil and having to deal with TH1 is a challenge for me. Sorry for my shortcomings. And thanks for these hints. This quoting makes indeed the intent of the regexp much more clear. I included that in the branch.

The branch now contains fixes for the injection issue. Are there other concerns?

If no objections are raised I would again consider merging into trunk. So please stop me if you see problems.

The branch now contains fixes for the injection issue. Are there other concerns?

FWIW, none here.

Now on the th1-taint branch I have implemented enhancements to TH1 that attempt to automatically detect potential XSS and SQL-injection vulnerabilities. The current state of affairs:

1. Within TH1, string and variable values that can potentially be controlled by a non-admin user are marked as "tainted" internally.

2. If you generate HTML text using the "html" TH1 command, or if you run SQL using the "query" TH1 command and the text you output or the SQL you are trying to run is tainted, then a warning message is written to the error log.

   • The original design was to panic. But I thought that might break too much legacy.
   • We could add a "strict-th1" setting or a "strict" TH1 command that forces a panic or other fatal error if a tainted value is used in an unsafe way.

3. I had to make some adjustments to the default ticket configuration because it was using "html" instead of "puts" to output some date/time strings. This is technically safe to do, but the new tainted string mechanism doesn't know that so it was generating lots of warnings.

Here's How You Can Help:

• Test the current branch on your configurations, after enabling error logging. Are you safe as configured? Do you need to make adjustments?
• Suggest new safety checks to install.
• How can we roll out this security enhancement with minimal disruption to legacy deployments?
• Should I merge this to trunk and/or put it on the main server and see what happens?

Perl taint mode, nice!

I am currently visiting family for easter, so my testing might be limited. Will be able to do better in Tuesday.

I get the new warning for the repository title on each visited page, I guess the TH1 generating the page header has to be adapted.

I got an segfault with the branch which isn't happening on trunk.

I served locally with:

fossil serve --repolist repositories/ --errorlog fossil-test.log

It happens on creating a new ticket, I only add a title and submit and get the error. Errorlog reports:

------------- 2025-04-19 20:58:34 UTC ------------
panic: Segfault during /tktnew in fossil [5d17ced68d]
Backtrace: fossil(+0x182e50) [0x558249cabe50] /usr/lib/libc.so.6(+0x40110) [0x7fdbeaf68110] fossil(+0x219f61) [0x558249d42f61] fossil(+0x110f77) [0x558249c39f77] fossil(+0x1117f3) [0x558249c3a7f3] fossil(+0x113e3c) [0x558249c3ce3c] fossil(+0x110f77) [0x558249c39f77] fossil(+0x1117f3) [0x558249c3a7f3] fossil(+0x2105ca) [0x558249d395ca] fossil(+0x21c0a6) [0x558249d450a6] fossil(+0x185756) [0x558249cae756] fossil(+0x187588) [0x558249cb0588] fossil(+0x18433a) [0x558249cad33a] fossil(+0x2f006) [0x558249b58006] /usr/lib/libc.so.6(+0x2abfc) [0x7fdbeaf52bfc] /usr/lib/libc.so.6(__libc_start_main+0x85) [0x7fdbeaf52cb5] fossil(+0x2f031) [0x558249b58031] (pid 10951)
while in /tktnew
HTTP_HOST=localhost:8080
HTTP_REFERER=http://localhost:8080/test/tktnew
HTTP_USER_AGENT=Mozilla/5.0 (X11; Linux x86_64; rv:137.0) Gecko/20100101 Firefox/137.0
PATH_INFO=tktnew
REMOTE_ADDR=127.0.0.1
REQUEST_METHOD=POST
REQUEST_URI=/test/tktnew
SCRIPT_NAME=/test

We could add a "strict-th1" setting or a "strict" TH1 command that forces a panic or other fatal error if a tainted value is used in an unsafe way.

My vote would be the setting. From the standpoint of the repository admin it is more useful to enable it globally (for the repo), than on a script-basis.

Also… a setting could be checked on the Security Audit page and generate a warning if disabled. Maybe with the hint that it may break custom scripts if enabled.

Taint-Mode Status Report

• The changes are now on trunk and are install on all servers I control.

• The vuln-report setting determines what happens when a potential XSS or SQL-injection vulnerability is seen. All repos on the main server are set to "block"

• I elected to omit the vuln-report setting from the Security-Audit, for now. I might add it at some point in the future.

Thanks for your work. And sorry to make such steps needed by stupidly carelessly introducing an injection bug in the first place.

On my repositories (with quite large amounts of modifications in the ticket setup - table def. & TH1 code), any use of /tktview (and therefore /info) and /tktedit and /tktnew immediately fails with panic segfaults:

panic: Segfault during /tktview in fossil [2116238e80]
Backtrace: ./fossil_2116238e80cc3dcb() [0x55466b] /lib64/libc.so.6(+0x4e5b0) [0x152942a4f5b0] ./fossil_2116238e80cc3dcb() [0x4dd2e2] ./fossil_2116238e80cc3dcb() [0x4dfad3] ./fossil_2116238e80cc3dcb() [0x4dd4ee] ./fossil_2116238e80cc3dcb() [0x4ddb18] ./fossil_2116238e80cc3dcb() [0x5dbaf6] ./fossil_2116238e80cc3dcb() [0x4dd4ee] ./fossil_2116238e80cc3dcb() [0x4ddb18] ./fossil_2116238e80cc3dcb() [0x4df847] ./fossil_2116238e80cc3dcb() [0x4dbe42] ./fossil_2116238e80cc3dcb() [0x4de878] ./fossil_2116238e80cc3dcb() [0x4dd4ee] ./fossil_2116238e80cc3dcb() [0x4dc03b] ./fossil_2116238e80cc3dcb() [0x4dc3c5] ./fossil_2116238e80cc3dcb() [0x4dd479] ./fossil_2116238e80cc3dcb() [0x4ddb18] ./fossil_2116238e80cc3dcb() [0x5e5459] ./fossil_2116238e80cc3dcb() [0x556e41] ./fossil_2116238e80cc3dcb() [0x5589e8] (pid 3224228)
while in /tktview
HTTP_HOST=localhost:8080
HTTP_USER_AGENT=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36 Edg/135.0.0.0
PATH_INFO=/tktview/cd1d6546a9b31975
REMOTE_ADDR=127.0.0.1
REQUEST_METHOD=GET
REQUEST_URI=/tktview/cd1d6546a9b31975

Tried with versions 2116238e80cc3dcb (just after the merge of th1-taint) and 9e035ee3b3a2e473 (the active one on fossil-smc), same problem. The one before the merge works without any problems so far f1db9ead1d44290a

Some manual bisecting: The first commit in the branch th1-taint b0b44924805848ec still works, starting with d1bb87bcfdd4ad32 I do see these segfaults.

Bisecting is not helpful for this problem. Better is to recompile Fossil manually using CFLAGS='-O0 -g' and then run it in a debugger to see where the failure is occurring. You can create a "request" file (call it "r1.txt") something like this:

GET /tktview HTTP/1.0

Then run Fossil in a debugger with a command like this:

fossil test-http <r1.txt

The debugger should show use exactly where the problem is occurring. If you send me a stack trace (that has symbolic names and line numbers) that might be enough of a hint for me to fix it.

The other thing you can try is just sending me your "View Ticket Page" script.

ok, will try that.

Have to learn how to run something in a debugger (gdb?) and piping command line arguments into it ...

Otherwise, I will have to do some work stripping the view-ticket script down, it is huge (and depends on some proc defined in ticket-common - which might be the problem?)

Is the TH1-taint stuff available in the TH1-Testpage (admin)? Might check some of the procs there first.

You can use the "fossil test-th-eval" command or similar to test your scripts, yes.

If you run "fossil config export ticket to-drh.txt" and then send the to-drh.txt file to my private email, I'm guessing I will be able to diagnose and fix the problem quickly.

The lappend command was segfaulting if any of its arguments were tainted. That problem is fixed by check-in 2025-04-22T17:40Z.

crossposted, you figured it out without my input. Impressed!

Do I read this correctly as saying that everything is now working for you, not that you have upgraded to the latest trunk check-in? Can we close out your issue now?

No, just that by the time I wrote down my findings, I was surprized you identified lappend as well. Though latest trunk (6476f287d3) now fails differently with my example from above:

string too large. maximum size 286MB.

or even simpler, no unsafe query, just lappend in query-code-block (and on fossils default repo itself, just on /admin_th1):

set res {}
query {select status as v from ticket} {lappend res "$v"}

string too large. maximum size 286MB.

You need to upgrade to check-in 2025-04-22T19:34Z or later to fix another lappend bug.

Yes, the query string is "unsavory", but TH1 figures out that the $fld value that you are sticking into the middle of the query text is uncontaminated by user input (the value is not tainted) so it is allowed.

confirmed, thank you very much!

Works fine so far when using aa66767bac78b2b0.

Now up to eliminating those html $var things in my TH1 code that trigger the XSS warning, my job ...