TCP ISN CPU Information Leak Protection - tirdad

Patrick · November 25, 2019, 12:31pm

An analysis of TCP secure SN generation in Linux and its privacy issues
Tirdad kernel module for random ISN generation
Tor Project bug report: Add research idea for Linux TCP Initial Sequence Numbers may aid correlation
research paper: Hot or not: revealing hidden services by their clock skew
Whonix ticket

Patrick · November 28, 2019, 12:43pm

https://github.com/0xsirus/tirdad/pull/2

Patrick · November 28, 2019, 5:42pm

Packing is done. Available from all Whonix repositories. Testers wanted! To install:

sudo apt update
sudo apt install tirdad

Description and package source code:

Patrick · November 29, 2019, 1:22pm

Enhanced loader. More sanity tests.
Use /dev/random instead of openssl (/dev/urandom as per /dev/random vs. /dev/urandom - #2 by HulaHoop).
Code simplification.

Comparing 0.1.1-1...0.1.2-1 · Kicksecure/tirdad · GitHub

Patrick · January 1, 2020, 9:06am

github.com/systemd/systemd

/etc/modprobe.d install option broken - cgroup: fork rejected by pids controller in /system.slice/systemd-modules-load.service

opened 08:46AM - 01 Jan 20 UTC

closed 06:54PM - 31 Aug 20 UTC

adrelanos

needs-reporter-feedback ❓ modules-load

**systemd version the issue has been seen with** 241 **Used distribution**… Debian buster. **Expected behaviour you didn't see** Kernel module correctly load. **Unexpected behaviour you saw** Kernel module not load. **Steps to reproduce the problem** Install [tirdad](https://github.com/Whonix/tirdad) (probably also any other kernel module). file `/etc/modprobe.d/30_tirdad.conf` install tirdad /usr/lib/tirdad/loader (modprobe.d supports `install modulename command...` to run arbitrary command as per its [man page](https://linux.die.net/man/5/modprobe.d). file `/usr/lib/modules-load.d/30_tirdad.conf` tirdad Module won't load at boot. sudo systemctl restart systemd-modules-load > Jan 01 08:36:17 host systemd[1]: Stopped Load Kernel Modules. > Jan 01 08:36:17 host systemd[1]: Stopping Load Kernel Modules... > Jan 01 08:36:17 host systemd[1]: Starting Load Kernel Modules... > Jan 01 08:36:36 host systemd-modules-load[7529]: /usr/lib/tirdad/loader: fork: retry: Resource temporarily unavailable > Jan 01 08:36:36 host kernel: cgroup: fork rejected by pids controller in /system.slice/systemd-modules-load.service > Jan 01 08:36:37 host systemd-modules-load[7529]: /usr/lib/tirdad/loader: fork: retry: Resource temporarily unavailable > Jan 01 08:36:39 host systemd-modules-load[7529]: sh: 1: Cannot fork > Jan 01 08:36:39 host systemd-modules-load[7529]: modprobe: ERROR: ../libkmod/libkmod-module.c:979 command_do() Error running install command for tirdad > Jan 01 08:36:39 host systemd-modules-load[7529]: modprobe: ERROR: could not insert 'tirdad': Unknown symbol in module, or unknown parameter (see dmesg) > Jan 01 08:36:39 host systemd-modules-load[7529]: modprobe: ERROR: ../libkmod/libkmod-module.c:979 command_do() Error running install command for tirdad > Jan 01 08:36:39 host systemd-modules-load[7529]: Inserted module 'tirdad' > Jan 01 08:36:39 host systemd[1]: Started Load Kernel Modules. cat /lib/systemd/system/systemd-modules-load.service ``` # SPDX-License-Identifier: LGPL-2.1+ # # This file is part of systemd. # # systemd is free software; you can redistribute it and/or modify it # under the terms of the GNU Lesser General Public License as published by # the Free Software Foundation; either version 2.1 of the License, or # (at your option) any later version. [Unit] Description=Load Kernel Modules Documentation=man:systemd-modules-load.service(8) man:modules-load.d(5) DefaultDependencies=no Conflicts=shutdown.target Before=sysinit.target shutdown.target ConditionCapability=CAP_SYS_MODULE ConditionDirectoryNotEmpty=|/lib/modules-load.d ConditionDirectoryNotEmpty=|/usr/lib/modules-load.d ConditionDirectoryNotEmpty=|/usr/local/lib/modules-load.d ConditionDirectoryNotEmpty=|/etc/modules-load.d ConditionDirectoryNotEmpty=|/run/modules-load.d ConditionKernelCommandLine=|modules-load ConditionKernelCommandLine=|rd.modules-load [Service] Type=oneshot RemainAfterExit=yes ExecStart=/lib/systemd/systemd-modules-load TimeoutSec=90s ```

github.com/0xsirus/tirdad

LKRG Compatibility / abolish need for userspace loader / /proc info file when initialization succeed

opened 09:06AM - 01 Jan 20 UTC

closed 08:27AM - 20 Jan 20 UTC

adrelanos

tirdad and [LKRG](https://www.whonix.org/wiki/Linux_Kernel_Runtime_Guard_LKRG) a…re currently incompatible. The tirdad module should load before LKRG. This is because LKRG detects modifications made by tirdad as a compromise of kernel integrity. Loading tirdad using the standard way with `/lib/modules-load.d/` isn't possible because it requires its userspace loader. If the loader would not be required, perhaps `/lib/modules-load.d/` in combination with `/etc/modprobe.d` [settings for Linux module dependencies](https://stackoverflow.com/questions/29717761/how-do-i-define-dependency-among-kernel-modules) could use soft dependencies. LKRG using `MODULE_SOFTDEP("post: module2")` or tirdad using `MODULE_SOFTDEP("pre: p_lkrg")`. `/etc/modprobe.d` feature to run arbitrary commands (the loader) through `/etc/modprobe.d` syntax `install modulename command` in combination with `/lib/modules-load.d/` also does not work due to perhaps systemd issue. -> https://github.com/systemd/systemd/issues/14459 I've also attempted to not use `/lib/modules-load.d/` / `/etc/modprobe.d` at all and load both kernel modules, tirdad and LKRG through systemd unit files. First start tirdad. And tirdad systemd unit file saying `Before=lkrg-dkms.service` and LKRG systemd unit file saying `After=tirdad.service`. That doesn't work either. Systemd is "too fast". After running `modprobe` the script finishes and it is considered done by systemd (how could it know any better). But tirdad needs a split second to finish initialization. (`Installing tirdad hook succeeded.`) LKRG runs before that, then seeing the kernel modification and then complaining about that. There is currently no clean way to check when `Installing tirdad hook succeeded.` is done. Well, the LKRG loader could parse the output of `dmesg` or tirdad loader could do that and until `Installing tirdad hook succeeded.` happened but that is a quite hacky solution. Long story short. Possible solutions: * Could you please consider removing need for any user space loader? Figure out random key and kernel address form inside the kernel module? (preferred) * And status file in `/proc` (or `/sys`?) creating when `Installing tirdad hook succeeded.` happened?

Patrick · January 1, 2020, 10:55am

Patrick · February 1, 2020, 12:34pm

Patrick · February 1, 2020, 12:37pm

Why should tirdad be loaded as early as possible? Currently tirdad is loaded before networking comes up through systemd-modules-load.service.

anontor · February 1, 2020, 4:27pm

Verified in the logs, it shows tirdad loads before sysinit. This is long before even networking-pre is reached. Systemd begins, and then almost immediately after, tirdad (and a few other modules) are inserted.
Since tirdad’s sole concern is the randomization of the ISN, as long as it starts before a network connection is established there is no issue (which it does) I do not think having it start any earlier than it does gives any advantage or benefit.

madaidan · February 5, 2020, 9:33pm

Just to make sure TCP ISNs are always random no matter what.

Root could undo that though which isn’t good for untrusted root.

madaidan · February 5, 2020, 9:45pm

Also, compiling tirdad in the kernel source tree will cause the module to be signed with CONFIG_MODULE_SIG_ALL so we don’t need any dkms hooks for it or anything.

Or, compiling it as built-in will make it not need to be signed at all.

If the same can be done for LKRG, only vbox additions will be left.

Patrick · February 6, 2020, 10:17am

Root might indeed install some package which then breaks
systemd-modules-load.service or something.

Patrick · February 14, 2020, 8:39am

Patrick · February 14, 2020, 10:34am

There is a minor issue, unwanted confusing error message related to systemd-modules-load.service / /usr/lib/modules-load.d/30_tirdad.conf.

Setting up linux-image-4.19.0-8-amd64 (4.19.98-1) …
I: /vmlinuz is now a symlink to boot/vmlinuz-4.19.0-8-amd64
I: /initrd.img is now a symlink to boot/initrd.img-4.19.0-8-amd64
/etc/kernel/postinst.d/30_remove-system-map:
Deleting system.map files…
removed ‘/boot/System.map-4.19.0-8-amd64’
Done. Success.
/etc/kernel/postinst.d/dkms:
Job for systemd-modules-load.service failed because the control process exited with error code.
See “systemctl status systemd-modules-load.service” and “journalctl -xe” for details.
Job for systemd-modules-load.service failed because the control process exited with error code.
See “systemctl status systemd-modules-load.service” and “journalctl -xe” for details.

After APT finished however tirdad is properly installed and systemd-modules-load.service status is OK too.

It is this DKMS bug:

github.com/dell/dkms

Revert "Make newly installed modules available immediately"

dell:master ← seblu:fs54481

opened 02:01PM - 08 Jul 17 UTC

seblu

+0 -6

This reverts commit f5bfb12fef1fc06e56355cdba500eaa98d4e6aa8. Modules install…ation should install modules, not load them. It creates issues when you build modules for non running kernels or when you do reload in a middle of a bunch of modules rebuild. Moreover Arch Linux is not doing modules automatically. See https://bugs.archlinux.org/task/54481

Added a comment:

Revert "Make newly installed modules available immediately" by seblu · Pull Request #27 · dell/dkms · GitHub

Patrick · February 14, 2020, 10:37am

github.com/0xsirus/tirdad

tirdad module autoloading sensible?

opened 10:36AM - 14 Feb 20 UTC

closed 10:39AM - 15 Feb 20 UTC

adrelanos

[This DKMS issue](https://forums.whonix.org/t/tcp-isn-cpu-information-leak-prote…ction-tirdad/8552/14) made me wonder if loading tirdad using `systemd-modules-load.service` / [`/usr/lib/modules-load.d/30_tirdad.conf`](https://github.com/Whonix/tirdad/blob/master/debian/30_tirdad.conf) is the right approach. Other modules (such as battery, ext4, not sure if good example and probably many others) are automatically load on some event. Would it be doable, sensible to use the kernel's auto load mechanism to load tirdad too?

Patrick · February 17, 2023, 12:42pm

github.com/QubesOS/qubes-issues

sys-whonix and anon-whonix wont start with latest kernel 6.1.7-1.fc32

opened 10:15AM - 14 Feb 23 UTC

ihateprogramming88

T: bug C: kernel C: Whonix P: default needs diagnosis updates testing

[How to file a helpful issue](https://www.qubes-os.org/doc/issue-tracking/) #…## Qubes OS release (if applicable) xen_version : 4.14.5 linux 6.1.7-1.fc32.qubesx86_64 ### Brief summary With the latest kernel 6.1.7-1.fc32 sys-whonix and anon-whonix wont start, this is the error that i get. "qrexec-daemon startup failed qrexec-daemon.c:135sigchld_parent_handler: connection to VM failed" not sure if this is a bug or something else. I thought it was best to report it.

Patrick · July 13, 2023, 1:30pm

github.com/0xsirus/tirdad

chatgpt review

opened 03:29AM - 13 Jul 23 UTC

closed 04:05AM - 13 Jul 23 UTC

adrelanos

chatgpt (AI) shows several things. Prompt was: > Are there any bugs in the fo…llowing code? > `<code pasted here>` reply was: > The provided code appears to be a Linux kernel module for hooking and modifying TCP Initial Sequence Number (ISN) generation functions. While I cannot guarantee the absence of all bugs without testing the code in a specific environment, I can identify some potential issues and concerns in the code: > > Header inclusion: Some header files included in the code (<asm/cacheflush.h>, <asm/uaccess.h>, <linux/in6.h>) may not be necessary or may not exist in certain kernel versions. It is important to ensure that the required headers are included correctly. > > Inconsistent header inclusion: The header file <linux/kernel.h> is included twice in the code, which is unnecessary and can be removed. > > Macro redefinition: The macro CNORM is defined twice, which will cause a redefinition error during compilation. The second definition should be removed. > > Typo in comment: In the comment block near the struct target_vals definition, "tread" should be "treat." > > Inconsistent function naming: The store_p_bits function is named using underscores (_), while the other functions use a combination of underscores and hyphens (-). It is good practice to use consistent naming conventions for functions. > > Memory corruption risk: The memcpy calls in the install_hook_on function assume that the target address is valid and has enough space to copy FUSION_SIZE bytes. Without proper validation, there is a risk of memory corruption or buffer overflow. > > Unsigned type usage: In several places, the code explicitly casts variables to unsigned long or unsigned char. It is generally better to use the appropriate types directly to avoid potential issues. > > Error handling: The code lacks comprehensive error handling. It returns -1 in some cases, but there is no consistent error reporting mechanism or handling of these errors. > > Lack of comments: The code could benefit from more comments to explain the purpose and logic behind certain operations. > > It is crucial to thoroughly test and review the code in the specific environment and kernel version for which it is intended. Kernel modules directly interact with the system's core, and any bugs or errors in such modules can lead to system instability or security vulnerabilities. next prompt was: > Please fix these bugs. I will send a pull request for that.

github.com/0xsirus/tirdad

Refactor and Address Bugs in Tirdad TCP ISN Generator Hook

0xsirus:master ← adrelanos:chatgpt

opened 03:31AM - 13 Jul 23 UTC

adrelanos

+284 -311

This commit refactors and addresses several bugs in the Tirdad TCP ISN generator… hook code. The following changes have been made: Fixed header inclusion: Removed unnecessary or non-existent header files and resolved duplicated inclusion of <linux/kernel.h>. Updated function naming: Ensured consistent naming conventions for functions by using underscores throughout. Fixed memory corruption risk: Added validation checks to ensure that target addresses are valid and have sufficient space for copying. Fixed unsigned type usage: Removed unnecessary explicit type casts and used appropriate types directly. Improved error handling: Added more comprehensive error reporting and handling mechanisms. Enhanced comments: Added explanatory comments to clarify the purpose and logic of certain operations. Other minor improvements: Cleaned up code formatting, addressed macro redefinition, and fixed a typo in a comment. fixes https://github.com/0xsirus/tirdad/issues/17

nurmagoz · October 6, 2024, 6:52pm

[user ~]% sudo apt -t bookworm-backports install linux-image-$(dpkg --print-architecture) linux-headers-$(dpkg --print-architecture)
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following additional packages will be installed:
  linux-headers-6.10.11+bpo-amd64 linux-headers-6.10.11+bpo-common
  linux-image-6.10.11+bpo-amd64 linux-kbuild-6.10.11+bpo linux-libc-dev
Suggested packages:
  linux-doc-6.10 debian-kernel-handbook
The following NEW packages will be installed:
  linux-headers-6.10.11+bpo-amd64 linux-headers-6.10.11+bpo-common
  linux-image-6.10.11+bpo-amd64 linux-kbuild-6.10.11+bpo
The following packages will be upgraded:
  linux-headers-amd64 linux-image-amd64 linux-libc-dev
3 upgraded, 4 newly installed, 0 to remove and 80 not upgraded.
Need to get 117 MB of archives.
After this operation, 175 MB of additional disk space will be used.
Do you want to continue? [Y/n] y
Get:1 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-headers-6.10.11+bpo-common all 6.10.11-1~bpo12+1 [10.6 MB]
Get:2 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-image-6.10.11+bpo-amd64 amd64 6.10.11-1~bpo12+1 [101 MB]
Get:3 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-kbuild-6.10.11+bpo amd64 6.10.11-1~bpo12+1 [1,146 kB]
Get:4 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-headers-6.10.11+bpo-amd64 amd64 6.10.11-1~bpo12+1 [1,437 kB]
Get:5 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-headers-amd64 amd64 6.10.11-1~bpo12+1 [1,412 B]
Get:6 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-image-amd64 amd64 6.10.11-1~bpo12+1 [1,476 B]
Get:7 tor+https://deb.debian.org/debian bookworm-backports/main amd64 linux-libc-dev all 6.10.11-1~bpo12+1 [2,400 kB]
Fetched 117 MB in 1min 1s (1,907 kB/s)                                         
Selecting previously unselected package linux-headers-6.10.11+bpo-common.
(Reading database ... 120405 files and directories currently installed.)
Preparing to unpack .../0-linux-headers-6.10.11+bpo-common_6.10.11-1~bpo12+1_all
.deb ...
Unpacking linux-headers-6.10.11+bpo-common (6.10.11-1~bpo12+1) ...
Selecting previously unselected package linux-image-6.10.11+bpo-amd64.
Preparing to unpack .../1-linux-image-6.10.11+bpo-amd64_6.10.11-1~bpo12+1_amd64.
deb ...
Unpacking linux-image-6.10.11+bpo-amd64 (6.10.11-1~bpo12+1) ...
Selecting previously unselected package linux-kbuild-6.10.11+bpo.
Preparing to unpack .../2-linux-kbuild-6.10.11+bpo_6.10.11-1~bpo12+1_amd64.deb .
..
Unpacking linux-kbuild-6.10.11+bpo (6.10.11-1~bpo12+1) ...
Selecting previously unselected package linux-headers-6.10.11+bpo-amd64.
Preparing to unpack .../3-linux-headers-6.10.11+bpo-amd64_6.10.11-1~bpo12+1_amd6
4.deb ...
Unpacking linux-headers-6.10.11+bpo-amd64 (6.10.11-1~bpo12+1) ...
Preparing to unpack .../4-linux-headers-amd64_6.10.11-1~bpo12+1_amd64.deb ...
Unpacking linux-headers-amd64 (6.10.11-1~bpo12+1) over (6.1.112-1) ...
Preparing to unpack .../5-linux-image-amd64_6.10.11-1~bpo12+1_amd64.deb ...
Unpacking linux-image-amd64 (6.10.11-1~bpo12+1) over (6.1.112-1) ...
Preparing to unpack .../6-linux-libc-dev_6.10.11-1~bpo12+1_all.deb ...
Unpacking linux-libc-dev (6.10.11-1~bpo12+1) over (6.1.112-1) ...
Setting up linux-libc-dev (6.10.11-1~bpo12+1) ...
Setting up linux-image-6.10.11+bpo-amd64 (6.10.11-1~bpo12+1) ...
I: /vmlinuz.old is now a symlink to boot/vmlinuz-6.1.0-26-amd64
I: /initrd.img.old is now a symlink to boot/initrd.img-6.1.0-26-amd64
I: /vmlinuz is now a symlink to boot/vmlinuz-6.10.11+bpo-amd64
I: /initrd.img is now a symlink to boot/initrd.img-6.10.11+bpo-amd64
/etc/kernel/postinst.d/30_remove-system-map:
INFO: Deleting system.map files...
INFO: removed '/boot/System.map-6.10.11+bpo-amd64'
INFO: Done. Success.
/etc/kernel/postinst.d/dkms:
dkms: running auto installation service for kernel 6.10.11+bpo-amd64.
Sign command: /lib/modules/6.10.11+bpo-amd64/build/scripts/sign-file
Signing key: /var/lib/dkms/mok.key
Public certificate (MOK): /var/lib/dkms/mok.pub

Building module:
Cleaning build area...
make -j2 KERNELRELEASE=6.10.11+bpo-amd64 all...(bad exit status: 2)
Error! Bad return status for module build on kernel: 6.10.11+bpo-amd64 (x86_64)
Consult /var/lib/dkms/tirdad/0.1/build/make.log for more information.
Error! One or more modules failed to install during autoinstall.
Refer to previous errors for more information.
dkms: autoinstall for kernel: 6.10.11+bpo-amd64 failed!
run-parts: /etc/kernel/postinst.d/dkms exited with return code 11
dpkg: error processing package linux-image-6.10.11+bpo-amd64 (--configure):
 installed linux-image-6.10.11+bpo-amd64 package post-installation script subpro
cess returned error exit status 1
Setting up linux-headers-6.10.11+bpo-common (6.10.11-1~bpo12+1) ...
Setting up linux-kbuild-6.10.11+bpo (6.10.11-1~bpo12+1) ...
dpkg: dependency problems prevent configuration of linux-headers-6.10.11+bpo-amd
64:
 linux-headers-6.10.11+bpo-amd64 depends on linux-image-6.10.11+bpo-amd64 (= 6.1
0.11-1~bpo12+1) | linux-image-6.10.11+bpo-amd64-unsigned (= 6.10.11-1~bpo12+1); 
however:
  Package linux-image-6.10.11+bpo-amd64 is not configured yet.
  Package linux-image-6.10.11+bpo-amd64-unsigned is not installed.

dpkg: error processing package linux-headers-6.10.11+bpo-amd64 (--configure):
 dependency problems - leaving unconfigured
dpkg: dependency problems prevent configuration of linux-headers-amd64:
 linux-headers-amd64 depends on linux-headers-6.10.11+bpo-amd64 (= 6.10.11-1~bpo
12+1); however:
  Package linux-headers-6.10.11+bpo-amd64 is not configured yet.

dpkg: error processing package linux-headers-amd64 (--configure):
 dependency problems - leaving unconfigured
dpkg: dependency problems prevent configuration of linux-image-amd64:
 linux-image-amd64 depends on linux-image-6.10.11+bpo-amd64 (= 6.10.11-1~bpo12+1
); however:
  Package linux-image-6.10.11+bpo-amd64 is not configured yet.

dpkg: error processing package linux-image-amd64 (--configure):
 dependency problems - leaving unconfigured
Processing triggers for security-misc (3:39.9-1) ...
INFO: triggered security-misc: 'security-misc' security-misc DPKG_MAINTSCRIPT_NA
ME: 'postinst' $\@: 'triggered /usr' 2: '/usr'
/usr/libexec/security-misc/mmap-rnd-bits: INFO: Successfully written ASLR map co
nfig file:
/etc/sysctl.d/30_security-misc_aslr-mmap.conf
Running SUID Disabler and Permission Hardener... See also:
https://www.kicksecure.com/wiki/SUID_Disabler_and_Permission_Hardener
/var/lib/dpkg/info/security-misc.postinst: INFO: running: permission-hardener en
able
permission-hardener: [NOTICE]: To compare the current and previous permission mo
des, install 'meld' (or preferred diff tool) for comparison of file mode changes
:
    sudo apt install --no-install-recommends meld
    meld /var/lib/permission-hardener/existing_mode/statoverride /var/lib/permis
sion-hardener/new_mode/statoverride
/var/lib/dpkg/info/security-misc.postinst: INFO: Permission hardening success.
Errors were encountered while processing:
 linux-image-6.10.11+bpo-amd64
 linux-headers-6.10.11+bpo-amd64
 linux-headers-amd64
 linux-image-amd64
E: Sub-process /usr/bin/dpkg returned an error code (1)
zsh: exit 100   sudo apt -t bookworm-backports install  
[user ~]%

Can confirm that using latest kernel from backports on fresh kicksecure doesnt go so well.

Patrick · October 7, 2024, 3:10am

reported upstream:

Patrick · October 7, 2024, 3:13am

security review tirdad.c →

github.com/0xsirus/tirdad

Security review of tirdad source code

opened 07:02PM - 06 Oct 24 UTC

ArrayBolt3

Hello! I'm a contributor to the Kicksecure and Whonix projects, and as part of m…y work I did a security review on the code of tirdad. I found some potential bugs and things that could be fixed up a bit, and wanted to share them here in case you found them useful. Items are listed in order from most worrying to least worrying. ## Safety of `get_secret` ```c siphash_key_t *get_secret(void){ u32 temp; temp = *((u32*)(&seq_secret.key[0])); temp>>=8; last_secret.key[0] += temp; temp = *((u32*)(&seq_secret.key[1])); temp>>=8; last_secret.key[1] += temp; return &last_secret; } ``` Is there a particular reason that tirdad incrementally modifies a previously generated SipHash key rather than just generating a new one on every iteration? Given that this code modifies the internal state of the CSPRNG, this code is worrying since in theory an attacker could derive information about the CSPRNG by looking at TCP ISNs unless this algorithm is proven secure. The Linux kernel's siphash documentation (`linux/Documentation/security/siphash.rst`) states that "[k]eys should always be generated from a cryptographically secure source of random numbers...", and the `net_secret_init` function used in `linux/net/core/secure_seq.c` just regenerates the key with `net_get_random_once` every time either of `secure_tcp_seq` or `secure_tcpv6_seq` is called. ## Manual patching with `install_hook_on`, `store_p_bits`, and `recover_one` I'm not an expert in Linux page table manipulation, so I can't tell how these functions work, but they definitely are doing a *lot* of complex, low-level work. Couldn't the kernel live patching API be used here instead? That would make things a lot simpler and potentially safer. ## Potentially dangerous use of `memset` in `hook_init` ```c seqv4.adr = 0; seqv6.adr = 0; memset(&seq_secret.key,0,AGGREGATE_KEY_SIZE); ``` Earlier in the module `AGGREGATE_KEY_SIZE` is defined to be `16`. This `memset` call assumes that `siphash_key_t` will only ever contain a `key` field, and that it will always be exactly 16 bytes long (two u64 values). As of Linux 6.1 both of these are true, but neither of them are guaranteed, and if either of them change, this code will very likely do things it wasn't intended to (partial initialization of memory or buffer overflow). Would `memset(&seq_secret, 0, sizeof(siphash_key_t));` work here instead? ## Potentially dangerous use of `strcat` in `_s_out` ```c void _s_out(u8 err, char *fmt, ...){ va_list argp; char msg_fmt[255]; if (err){ strcpy(msg_fmt,CRED"[!] TIRDAD: "CNORM); }else{ strcpy(msg_fmt,CGREEN"[-] TIRDAD: "CNORM); } strcat(msg_fmt,fmt); strcat(msg_fmt,"\n"); va_start(argp,fmt); vprintk(msg_fmt,argp); va_end(argp); } ``` Right now there's nothing in tirdad that makes this a problem, but `_s_out` assumes that whatever is passed into the `fmt` algorithm will fit in however much room there is left in `msg_fmt` after the addition of the tirdad "marker" at the start. The array is fixed-size and the `fmt` variable could be of any arbitrary length, so if a really big string is passed in (one dynamically generated, for instance), this will buffer overflow. Using `strncat` instead may resolve the problem. ## Confusing C macro `HANDLER` ```c #define SYMBOL_LOOKUP(s) (((u64 (*)(const char *))(kasln_adr))(s)) #define HANDLER(t,l,ret,...) t l ## h_hk(struct kprobe * kp, struct pt_regs * r\ __VA_OPT__(,) __VA_ARGS__){\ ret;\ } HANDLER(int,pre,return 0) HANDLER(void,post,return,unsigned long flags) ``` This is not exactly easy to read, and it's actually shorter to write the equivalent expanded code manually: ``` int preh_hk(struct kprobe *kp, struct pt_regs *r ) { return 0; } void posth_hk(struct kprobe *kp, struct pt_regs *r, unsigned long flags) { return; } ``` ## Ambiguous errno use ```c if (!seqv4.adr || !seqv6.adr){ _s_out(1,"FATAL: Name lookup failed."); return -1; //EPERM but we use it as a generic error number } ``` If this return value is used as an errno, it seems to me that using descriptive errno values would be worthwhile. Otherwise, I don't think the comment about EPERM is needed, it slightly confused me while reading the code. ## Weak randomness check after initial `get_random_bytes` call ```c get_random_bytes(&seq_secret.key,AGGREGATE_KEY_SIZE); for (i=0;i<32;i++){ if ( *( ((u8*)(&seq_secret.key)) + i ) !=0) break; } ``` This is a good sanity check, but if we're checking the output of `get_random_bytes` anyway it might be worthwhile to make sure it didn't return a whole bunch of identical non-zero bytes. ## Strict aliasing violation with type punning This isn't really a problem since Linux is built with `-fno-strict-aliasing`, but there's a lot of places in the code that get a pointer to a variable, cast it to an incompatible pointer type, and then dereference it. Generally unions are preferable when this kind of thing is necessary. Not sure if it's worth it to rewrite those parts, but I thought I'd mention it at least. That's what I found looking through it. Thanks for making this kernel module, it looks really useful. If I find the time, I'll try porting it to use kernel live patching and see if it still works that way.