bug(security): CVE-2025-53865 - XSS bug

Extensive fixes in devel, responsive templates known to be
exploitable.

Similar constructs in classic and minimal templates not known
to be exploitable, but changed anyway.

doc/upgrading.txt:

  Reformat to 66 characters.

  Update with assigned CVE number.

  Add section on fixing tal:replace with unsafe data.

  Document analysis and assumptions in comment in file.

doc/security.txt:

  Update with CVE number.

Author: rouilj

doc/security.txt

@@ -28,8 +28,8 @@ source release tarballs.
 CVE Announcements
 -----------------
 
-  * `CVE-2025-pending`_ - :ref:`XSS security issue with devel or
-    responsive templates <CVE-2025-pending>`. Fixed in release 2.5.0,
+  * `CVE-2025-53865`_ - :ref:`XSS security issue with devel or
+    responsive templates <CVE-2025-53865>`. Fixed in release 2.5.0,
     directions available for fixing trackers based on these templates.
 
   * `CVE-2024-39124`_ - :ref:`classhelpers (_generic.help.html) are
@@ -43,8 +43,8 @@ CVE Announcements
     executed. <CVE-2024-39126>` Fixed in release 2.4.0, directions
     available for fixing in prior versions.
 
-.. _CVE-2025-pending:
-        https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2025-pending
+.. _CVE-2025-53865:
+        https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2025-53865
 .. _CVE-2024-39124:
         https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-39124
 .. _CVE-2024-39125:

doc/upgrading.txt

@@ -108,24 +108,44 @@ your database.
 Migrating from 2.4.0 to 2.5.0
 =============================
 
-.. _CVE-2025-pending:
+.. _CVE-2025-53865:
 
 XSS security issue with devel and responsive templates (recommended)
 --------------------------------------------------------------------
 
+There are actually two different issues under this heading.
+
+  1. incorrect use of the ``structure`` keyword with
+     ``tal:content``
+  2. use of ``tal:replace`` on unsafe input
+
+In the discussion below, the :term:`html directory` means one or
+more directories listed in the ``templates`` key of your
+tracker's ``config.ini`` file.
+
+These directions can be used to solve the XSS security issue with
+any version of Roundup. Even if you used a classic or minimal
+template, you should check your trackers for these issues. The
+classic template fixed most of these many years ago, but the
+updates were not made to the devel and responsive templates. No
+report of similar issues with the jinja template has been seen.
+
+Incorrect use of structure in templates
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 The devel and responsive templates prior to Roundup 2.5 used this
 construct::
 
   tal:content="structure context/MUMBLE/plain"
 
 Where ``MUMBLE`` is a property of your issues (e.g. title).
 
-This construct allows a URL with a carefully crafted query parameter
-to execute arbitrary JavaScript.
+This construct allows a URL with a carefully crafted query
+parameter to execute arbitrary JavaScript.
 
-You should check all your trackers.  The classic template has not used
-this construct since at least 2009, but your tracker's templates may
-use the offending construct anyway.
+You should check all your trackers.  The classic template has not
+used this construct since at least 2009, but your tracker's
+templates may use the offending construct anyway.
 
 This fix will apply if your tracker is based on the responsive or
 devel template. Check the TEMPLATE-INFO.txt file in your tracker
@@ -138,53 +158,190 @@ field. For example a Name like::
 
 shows that tracker is based on the responsive or devel templates.
 
-.. _cve-2025-pending-fixed:
+.. _cve-2025-53865-fixed:
 
-To fix this, remove the ``structure`` declaration when it is used with
-a plain representation. So fixing the code by replacing the example
-above with::
+To fix this, remove the ``structure`` declaration when it is used
+with a plain representation. So fixing the code by replacing the
+example above with::
 
   tal:content="context/MUMBLE/plain"
 
 prevents the attack.
 
 To check for this issue, search for ``structure`` followed by
 ``/plain`` in all your html templates. If you are on a Linux/Unix
-system you can search the html subdirectory of your tracker with the
-following::
+system you can search the html subdirectory of your tracker with
+the following::
 
    grep 'structure.*/plain' *.html
 
 which should return any lines with issues.
 
 .. warning::
 
-   Backup the files in the ``html`` subdirectory of your tracker in case
-   an edit goes wrong.
+   Backup the files in the ``html`` subdirectory of your tracker
+   in case an edit goes wrong.
 
-You can fix this issue using the GNU sed command::
+As an example, you could fix this issue using the GNU sed
+command::
 
   sed -i.bak -e '/structure.*\/plain/s/structure.//' *.html
 
-to edit the files in place and remove the structure keyword. It will
-create a ``.bak`` file with the original contents of the file. If you
-are on windows, some text editors support search and replace using a
-regular expression.
+to edit the files in place and remove the structure keyword. It
+will create a ``.bak`` file with the original contents of the
+file. If your templates were changed, this might still miss some
+entries. If you are on windows, some text editors support search
+and replace using a regular expression.
 
 If the construct is split across lines::
 
      tal:content="structure
               context/MUMBLE/plain"
 
-the commands above will miss the construct. So you should also search
-the html files using ``grep /plain *.html`` and verify that all of the
-``context/MUMBLE/plain`` include ``tal:content`` as in the `fixed
-example above <#cve-2025-pending-fixed>`_. Any lines that have
-``context/MUMBLE/plain`` without ``tal:content=`` before it need to be
-manually verified/fixed.
+the commands above will miss the construct. So you should also
+search the html files using ``grep /plain *.html`` and verify
+that all of the ``context/MUMBLE/plain`` include ``tal:content``
+as in the `fixed example above <#cve-2025-53865-fixed>`_. Any
+lines that have ``context/MUMBLE/plain`` without ``tal:content=``
+before it need to be manually verified/fixed.
 
 The distributed devel and responsive templates do not split the
-construct across lines, but if you changed the files it may be split.
+construct across lines, but if you changed the files it may be
+split.
+
+tal:replace used with unsafe input
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The problem was caused by the following markup::
+
+  <span tal:replace="context/MUMBLE" />
+
+in the head of the ``bug.item.html``, ``task.item.html`` and
+other files in the devel and responsive templates.
+
+This was fixed many years ago in the classic template's
+``index.item.html``. The classic template replaces the above
+construct with::
+
+  <tal:x tal:content="context/MUMBLE" />
+
+``tal:content`` explicitly escapes the result unless the
+``structure`` directive is used. ``tal:replace`` expects the
+result to be safe and usable in an HTML context.
+
+TAL drops any tags that it doesn't know about from the output.
+``<tal:x tal:content="..." />`` results in the value of the
+content expression without a surrounding html tag. (Effectively
+replacing the construct.)
+
+The following diff for ``bug.item.html`` in the devel template
+shows the change to make things safe (remove lines starting with
+``-`` and add lines staring with ``+``)::
+
+     <tal:block metal:use-macro="templates/page/macros/frame">
+     <title metal:fill-slot="head_title">
+     <tal:block condition="context/id" i18n:translate=""
+    - >Bug <span tal:replace="context/id" i18n:name="id"
+    - />: <span tal:replace="context/title" i18n:name="title"
+    - /> - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
+    + >Bug <tal:x tal:content="context/id" i18n:name="id"
+    + />: <tal:x tal:content="context/title" i18n:name="title"
+    + /> - <tal:x tal:content="config/TRACKER_NAME" i18n:name="tracker"
+     /></tal:block>
+     <tal:block condition="not:context/id" i18n:translate=""
+      >New Bug report - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
+
+A similar change was applied in the following html files in the
+devel or responsive templates:
+
+.. rst-class:: multicol
+
+* _generic.collision.html
+* bug.item.html
+* keyword.item.html
+* milestone.item.html
+* msg.item.html
+* task.item.html
+* user.item.html
+
+Also ``page.html`` should be changed from::
+
+   <p class="label"><b tal:replace="request/user/username">username</b></p>
+
+to::
+
+   <p class="label"><b tal:replace="python:request.user.username.plain(escape=1)">username</b></p>
+
+The code audit found the ``tal:replace`` construct is used with
+``context/id`` and ``context/designator`` paths. The references
+to these paths have been changed to use ``tal:x`` in the classic
+template's ``msg.item.html`` file and the classic and minimal
+template's ``_generic.collision.html`` file.
+
+These paths are critical to navigation in Roundup and are set
+from the path part of the URL. Roundup's URL path validation
+makes it unlikely that an attacker could exploit them. If you
+wish you can change your templates or copy the corresponding
+files from the template if you haven't made local changes.
+
+Also you may have used copies of these insecure templates
+elsewhere in your tracker (e.g. to create a feature class).  To
+find other possible issues you can use the command::
+
+  grep -r "tal:replace=" *.html
+
+in your tracker's :term:`html directory`. Check each occurrence
+and if needed, change it to the safer form. You should consider
+any reference to ``context`` to be under the user's (attacker's)
+control. Also ``db`` (excluding ``db/config``) and ``request``
+references that use user supplied content
+(e.g. ``request/user/username`` above) should be changed to
+``tal:x`` form
+
+.. comment:
+   As part of the analysis, the following command was used to find
+   potentially vulnerable stuff in the templates. Each grep -v was
+   removed to display items in that category and they were checked::
+
+     grep -r 'tal:replace' . | grep -v 'replace="batch' | \
+        grep -v 'replace="config' | grep -v 'replace="db/config' | \
+        grep -v 'replace="structure' | grep -v 'replace="python:' | \
+        grep -v 'replace="request/'
+
+
+   context/id, context/designator:
+     assume safe if used in an class.item.html page as the page
+     wouldn't be shown if they weren't valid numbers/designators.
+
+     Might not be ok referenced in a _generic fallback page though.
+
+   config, db/config, batch, nothing:
+     should be safe as they are not under user control
+
+   request/classname (python:request._classname), request/template:
+     should be safe as they are needed to navigate to a display page,
+     so if they are invalid nothing will be displayed.
+
+   utils, python:
+     assume it's written correctly and is safe (could use some new
+     tests for the shipped utility functions). The intent of these
+     can be to deliver blocks of <script> or other html markup.
+
+   db, request:
+     might be dangerous when accessing user supplied values.
+
+   request/user/username:
+     Escape these. If the username is an XSS issue, an attacker could
+     use it to compromise a user.
+
+   request/dispname:
+     should be quoted and is by the existing python: code.
+
+   Open question: why does there have to be an error generated by the
+   url @sort=1. Without invalid sort param, the exploit url doesn't
+   work and the context appears to use the database's title not the one
+   in the url. Also its not positional @sort=1 can appear anywhere in
+   the url.
 
 Deprecation Notices (required)
 ------------------------------

share/roundup/templates/classic/html/_generic.collision.html

@@ -11,6 +11,6 @@
   There has been a collision. Another user updated this node
   while you were editing. Please <a href='${context}'>reload</a>
   the node and review your edits.
-"><span tal:replace="context/designator" i18n:name="context" />
+"><tal:x tal:content="context/designator" i18n:name="context" />
 </td>
 </tal:block>

share/roundup/templates/classic/html/msg.item.html

@@ -2,7 +2,7 @@
 <tal:block metal:use-macro="templates/page/macros/icing">
 <title metal:fill-slot="head_title">
 <tal:block condition="context/id" i18n:translate=""
- >Message <span tal:replace="context/id" i18n:name="id"
+ >Message <tal:x tal:content="context/id" i18n:name="id"
  /> - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
 /></tal:block>
 <tal:block condition="not:context/id" i18n:translate=""

share/roundup/templates/devel/html/_generic.collision.html

@@ -11,6 +11,6 @@
   There has been a collision. Another user updated this node
   while you were editing. Please <a href='${context}'>reload</a>
   the node and review your edits.
-"><span tal:replace="context/designator" i18n:name="context" />
+"><tal:x tal:content="context/designator" i18n:name="context" />
 </td>
 </tal:block>

share/roundup/templates/devel/html/bug.item.html

@@ -1,9 +1,9 @@
 <tal:block metal:use-macro="templates/page/macros/frame">
 <title metal:fill-slot="head_title">
 <tal:block condition="context/id" i18n:translate=""
- >Bug <span tal:replace="context/id" i18n:name="id"
- />: <span tal:replace="context/title" i18n:name="title"
- /> - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
+ >Bug <tal:x tal:content="context/id" i18n:name="id"
+ />: <tal:x tal:content="context/title" i18n:name="title"
+ /> - <tal:x tal:content="config/TRACKER_NAME" i18n:name="tracker"
 /></tal:block>
 <tal:block condition="not:context/id" i18n:translate=""
  >New Bug report - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"

share/roundup/templates/devel/html/keyword.item.html

@@ -3,8 +3,8 @@
 >
 <title metal:fill-slot="head_title">
 <tal:if condition="context/id" i18n:translate=""
- >Keyword <span tal:replace="context/id" i18n:name="id"
- />: <span tal:replace="context/name" i18n:name="title"
+ >Keyword <tal:x tal:content="context/id" i18n:name="id"
+ />: <tal:x tal:content="context/name" i18n:name="title"
  /> - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
 /></tal:if>
 <tal:if condition="not:context/id" i18n:translate=""

share/roundup/templates/devel/html/milestone.item.html

@@ -2,7 +2,7 @@
 
 <tal:block metal:use-macro="templates/page/macros/frame">
   <tal:block metal:fill-slot="body_title">
-    <p class="header">Milestone<span tal:replace="context/id" /> Editing</p>
+    <p class="header">Milestone<tal:x tal:content="context/id" /> Editing</p>
   </tal:block>
 
   <tal:block metal:fill-slot="content">

share/roundup/templates/devel/html/msg.item.html

@@ -1,7 +1,7 @@
 <tal:block metal:use-macro="templates/page/macros/frame">
 <title metal:fill-slot="head_title">
 <tal:block condition="context/id" i18n:translate=""
- >Message <span tal:replace="context/id" i18n:name="id"
+ >Message <tal:x tal:content="context/id" i18n:name="id"
  /> - <span tal:replace="config/TRACKER_NAME" i18n:name="tracker"
 /></tal:block>
 <tal:block condition="not:context/id" i18n:translate=""

share/roundup/templates/devel/html/page.html

@@ -190,7 +190,7 @@ <h1><a href="/">Roundup Demo Tracker</a></h1>
       </form>
      </li>
      <li tal:condition="python:request.user.username != 'anonymous'" class="submenu">
-      <p class="label"><b tal:replace="request/user/username">username</b></p>
+      <p class="label"><b tal:replace="python:request.user.username.plain(escape=1)">username</b></p>
       <ul>
        <li>
         <a href="#" tal:attributes="href python:request.indexargs_url('bug', {