search

mach-kernel / cadius

A maintained fork of BrutalDeluxe's Cadius ProDOS disk imaging utility (used for making Apple II disk images).
GNU General Public License v3.0
31 stars 9 forks source link

REPLACEFILE buffer overflow #17

Closed sicklittlemonkey closed 6 years ago

sicklittlemonkey commented 6 years ago

This is why strdup() is considered harmful!

      char *prodos_file_name = strdup(param->prodos_folder_path);
...
        strcat(prodos_file_name, "/");
      strcat(prodos_file_name, file_name);

Would submit a PR but not sure how you want to handle it. Other code in the project is very conservative with filename/path lengths, eg:

   char prodos_folder_path[1024];

Also worth keeping comments up to date - should be REPLACEFILE:

  /** ADDFILE <2mg_image_path> <target_folder_path> <file_path> **/
  if(!my_stricmp(argv[1],"REPLACEFILE") && argc == 5)
    {
      param->action = ACTION_REPLACE_FILE;

Cheers, Nick.

mach-kernel commented 6 years ago

Good find! I'd be happy to receive a PR if you have it or if you'd like I can fix it with the other bug that was opened. 😸

sicklittlemonkey commented 6 years ago

I don't have a PR ready, and my allotted retro time for today is done!

It took me longer than I'd like to admit to find this. In a VS debug build the error would appear on cleanup in free()ing code, so I spent a couple of hours understanding the code and tracking back - until I realized it wasn't a free problem at all, and the likeliest place for a buffer overflow was the latest changes.