Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-111242

sdpscanner strcpy()s sdp_data_t::val::str into a fixed-size 1KiB buffer

XMLWordPrintable

    • a811bcb3e (dev), e673d5602 (6.5), eb75c50e8 (tqtc/lts-6.2), 092d5f8cb (tqtc/lts-5.15), c97efb48b (6.4)
    • Foundation Sprint 76

      In main.cpp, we have this code:

      #define BUFFER_SIZE 1024
      ~~~
      char snBuffer[BUFFER_SIZE];
      ~~~
          case SDP_URL_STR8:
          case SDP_URL_STR16:
          case SDP_URL_STR32:
              strncpy(snBuffer, data->val.str, data->unitSize - 1);
      

      This looks dangerous. The third argument should be sizeof snBuffer, not data->unitSize - 1. Even if by some technical reason strlen(val.str) can only be < 1024, it still looks wrong. If the idea was to avoid strncpy()'s filling of the whole buffer that way, strncat() would be a better choice (cf. https://codereview.qt-project.org/c/qt/qtbase/+/460921).

      I don't know the external constraints here, so I'm creating this ticket instead of fixin myself: SDP_TEXT_STR handling suggests that there may be embedded NULs, so xmlString.append(data->val.str, data->unitSize - 1) may be as wrong as assuming val.str is NUL-terminated, in which case xmlString.append(data->val.str) would be apt. The (original?) code at https://android.googlesource.com/platform/external/bluetooth/bluez/+/master/src/sdp-xml.c#318 is also weird, strndup()'ing instead of directly passing to appender()...

      Please check whether this is a security problem and fix accordingly.

        For Gerrit Dashboard: QTBUG-111242
        # Subject Branch Project Status CR V

            ivan.solovev Ivan Solovev
            mmutz Marc Mutz
            Vladimir Minenko Vladimir Minenko
            Alex Blasche Alex Blasche
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved:

                There are no open Gerrit changes