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

Details

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

    Description

      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.

      Attachments

        Issue Links

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

          Activity

            People

              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

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes