open5gs / open5gs

Open5GS is a C-language Open Source implementation for 5G Core and EPC, i.e. the core network of LTE/NR network (Release-17)
https://open5gs.org
GNU Affero General Public License v3.0
1.84k stars 762 forks source link

[Bug]: A potential stack overflow bug #3003

Closed shellwayxw closed 8 months ago

shellwayxw commented 8 months ago

Open5GS Release, Revision, or Tag

v2.7.0

Steps to reproduce

  1. Start open5gs-sgwud so the gtp server listens to 127.0.0.6:2152.
  2. Compile the attached poc and execute it. It sends a crafted message to gtp server and causes abort.

poc: sendto.txt

Logs

Open5GS daemon v2.7.0-74-g88a77f7+

02/22 13:40:54.480: [app] INFO: Configuration: '/home/test/open5gs/install/etc/open5gs/sgwu.yaml' (../lib/app/ogs-init.c:130)
02/22 13:40:54.480: [app] INFO: File Logging: '/home/test/open5gs/install/var/log/open5gs/sgwu.log' (../lib/app/ogs-init.c:133)
02/22 13:40:54.534: [pfcp] INFO: pfcp_server() [127.0.0.6]:8805 (../lib/pfcp/path.c:30)
02/22 13:40:54.534: [gtp] INFO: gtp_server() [127.0.0.6]:2152 (../lib/gtp/path.c:30)
02/22 13:40:54.534: [app] INFO: SGW-U initialize...done (../src/sgwu/app.c:31)
02/22 13:40:57.897: [gtp] ERROR: No length in the Extension header (../lib/gtp/util.c:77)
*** stack smashing detected ***: terminated
Aborted

Expected behaviour

The program should detect the overflow and not abort.

Observed Behaviour

With the stack canary in the compiler option, the program aborts.

eNodeB/gNodeB

No response

UE Models and versions

No response

shellwayxw commented 8 months ago

I checked the code and the root cause is that in ogs_gtpu_parse_header, variable i in line 66 is not checked. Adding i < OGS_GTP2_NUM_OF_EXTENSION_HEADER to the while loop (line 67) should resolve this problem.

acetcom commented 8 months ago

@shellwayxw

Could you please retest the modified code as shown below?

$ diff --git a/lib/gtp/util.c b/lib/gtp/util.c
index 3ec27d577..cb6b6e064 100644
--- a/lib/gtp/util.c
+++ b/lib/gtp/util.c
@@ -65,7 +65,8 @@ int ogs_gtpu_parse_header(
          * then the value of the Next Extension Header Type shall be 0. */

         i = 0;
-        while (*(ext_h = (((uint8_t *)gtp_h) + len - 1))) {
+        while (*(ext_h = (((uint8_t *)gtp_h) + len - 1)) &&
+                i < OGS_GTP2_NUM_OF_EXTENSION_HEADER) {
         /*
          * The length of the Extension header shall be defined
          * in a variable length of 4 octets, i.e. m+1 = n*4 octets,

Thanks a lot! Sukchan

shellwayxw commented 8 months ago

Sure. I applied the patch and retested the program. It did not abort again. Thanks for the fix.

acetcom commented 8 months ago

@shellwayxw

I've fixed this issue and pushed it to the main branch.

Please let me know if you have any other issues.

Thanks a lot! Sukchan