Bug Report: Graphics not masking

**myshkin** · June 19, 2011, 06:18

Originally posted by Blue Baron

Also, would ticket #1461 be fixed by removing the line "file_delete(old_savefile);" from line 380 and putting it on line 402? (The exsiting .old file is not deleted until the .new file is confirmed to have been written, but before the existing save file is renamed .old.) You can remove the lines " else file_delete(old_savefile);" on lines 414 and 415, if you want the .old file to remain as a backup.

No. The initially reported problem was that people were manually making copies of their savefiles, and the game was deleting them. The game should not be using any fixed filenames as temporary files.

Originally posted by Blue Baron

For ticket #942, I'm curious what the OSX and SDL ports use for icons?

Look at lib/xtra/icon/att*.png.

Originally posted by Blue Baron

Edit: for the first part of ticket #1469, adding the line "case VK_CLEAR: ch = '5'; kp=TRUE; break;" in handle_keydown() works for me. (wParam is 12 for me when pressing numpad5 when numlock is not on.)

Edit: For the second part of ticket #1469, the numpad keys are not handled in handle_keydown(), but WM_CHAR message are not generated for them, so they are not handled that way. ( I do not know where the plain numpad keypresses are handled.) Maybe in handle_keydown add lines like "case VK_NUMPAD7: if (mc||ma||ms) {ch = VK_NUMPAD7; kp = TRUE;} break;"

Strange. I guess the mapping of keypad 5 to clear does have some historical precedent. I know essentially nothing about the Windows port, and so will wait for someone else to comment, but this is quite encouraging.

**d_m** · June 19, 2011, 07:18

Blue Baron, you are my personal hero!

In case anyone else is interested, I fixed the PNG tilesets by opening them in gimp, saving a copy as PNG, turning off "save background color" and "save color value for transparent pixels".

For some reason (under WINE) I was able to walk/run/alter using the number pad with numlock on and off after applying your fix. Are you seeing problems still?

**Blue Baron** · June 19, 2011, 07:48

Originally posted by myshkin

No. The initially reported problem was that people were manually making copies of their savefiles, and the game was deleting them. The game should not be using any fixed filenames as temporary files.

so something like :

Code:

strnfmt(old_savefile, sizeof(old_savefile), "%s.old", path);
while (file_exists(old_savefile) && (count++ < 100))
{
    strnfmt(old_savefile, sizeof(old_savefile), "%s.%u%u.old", path, Rand_simple(32767), count);
}

except use the platform specific code where available? (file_exists has defines for platform specific code in z-file.c) err actually the Rand_simple function is defined in z-rand.h, but is not implemented. is there another random function that will not interfere with the game's rng?

Originally posted by myshkin

Look at lib/xtra/icon/att*.png.

Ahh thanks I did not know they were there and was only looking in the src directory.

**myshkin** · June 19, 2011, 07:57

Originally posted by Blue Baron

so something like :

Code:

strnfmt(old_savefile, sizeof(old_savefile), "%s.old", path);
while (file_exists(old_savefile) && (count++ < 100))
{
    strnfmt(old_savefile, sizeof(old_savefile), "%s.%u%u.old", path, Rand_simple(32767), count);
}

except use the platform specific code where available? (file_exists has defines for platform specific code in z-file.c) err actually the Rand_simple function is defined in z-rand.h, but is not implemented. is there another random function that will not interfere with the game's rng?

You have a good idea, but I prefer not writing independent security-related functions where possible. Alas, while most platforms have a reasonably secure function that opens a temporary file atomically, the function name and interface differ. My comments to the ticket indicate likely candidates for Unix-likes and Windows. I don't know what to do for nds.

**Blue Baron** · June 19, 2011, 08:46

Originally posted by d_m

For some reason (under WINE) I was able to walk/run/alter using the number pad with numlock on and off after applying your fix. Are you seeing problems still?

The plain numpad keys must be translated somewhere else. I was able to get the tunnelling working with numlock on by mapping the numpad keys to the characters used when numlock is off. But not running, probably where ever the plain keypresses are handled, trapping the WM_KEYDOWN message when shift is pressed, because I am not seeing a numpad keypress in handle_keydown when shift is pressed.

To say it another way, when numlock is on, handle_keydown() is seeing all of the numpad key presses except when shift is held down.

for the tunnelling I added lines like "case VK_NUMPAD7: if (mc||ma||ms) {ch = KC_HOME; kp = TRUE;} break;". When I had "ch = VK_NUMPAD7" in there various other commands were being executed, so the VK_ codes are can't be used directly by term_keypress().

Edit: It seems like with the shift key down, and num lock is on, windows is translating the numpad keys to the numpad keys with numlock off with shift off.

**d_m** · June 19, 2011, 14:26

Originally posted by Blue Baron

so something like :

Code:

strnfmt(old_savefile, sizeof(old_savefile), "%s.old", path);
while (file_exists(old_savefile) && (count++ < 100))
{
    strnfmt(old_savefile, sizeof(old_savefile), "%s.%u%u.old", path, Rand_simple(32767), count);
}

except use the platform specific code where available? (file_exists has defines for platform specific code in z-file.c) err actually the Rand_simple function is defined in z-rand.h, but is not implemented. is there another random function that will not interfere with the game's rng?

I think for this it's best to use mkstemp on Unix. I found a page with Unix-to-Windows conversion ideas:

http://www.suacommunity.com/dictionary/mkstemp-entry.php

It looks like _mktemp_s() might be the thing to us. I think this is the strategy that the real world uses nowadays.

**d_m** · June 19, 2011, 14:27

Originally posted by Blue Baron

The plain numpad keys must be translated somewhere else. I was able to get the tunnelling working with numlock on by mapping the numpad keys to the characters used when numlock is off. But not running, probably where ever the plain keypresses are handled, trapping the WM_KEYDOWN message when shift is pressed, because I am not seeing a numpad keypress in handle_keydown when shift is pressed.

Huh, this must be a place where WINE and Windows diverge

I was able to hook up a keyboard to my Linux netbook and saw all the numpad stuff working. Sigh.

**Blue Baron** · June 19, 2011, 16:55

Originally posted by d_m

Huh, this must be a place where WINE and Windows diverge

I was able to hook up a keyboard to my Linux netbook and saw all the numpad stuff working. Sigh.

I guess so

Shift numpad with numlock on did not run, but walked, in angband-ef5f426, angband 3.2.0, and Z+ for me. Does it work for anyone in windows in any version or variant?

Also, the special_key_list in 3.2.0 captures VK_SELECT, VK_PRINT, VK_EXECUTE,VK_SNAPSHOT,VK_HELP, and VK_16-VK_24, but handle_keydown() does not. Does anyone care about these keys?

Edit:

Originally posted by d_m

I think for this it's best to use mkstemp on Unix. I found a page with Unix-to-Windows conversion ideas:

http://www.suacommunity.com/dictiona...temp-entry.php

It looks like _mktemp_s() might be the thing to us. I think this is the strategy that the real world uses nowadays.

I understand the name collision part but not the security part. Is there anything I can read about it?

Anyways, above i meant something like:

Code:

#if (defined(POSIX)) // I do not know what should be here file_exists() uses #ifdef HAV_STAT
  strnfmt(old_savefile, sizeof(old_savefile),"%sXXXXXX", path);
  res =  mkstemp(old_saevfile);
#elif (defined(WINDOWS))
  strncopy(old_savefile, path,1024);
  res =  tmpnam_s(old_savefile, 1024); // or _mktmp_s()
  // However mkstemp opens a file descriptor, but these two don't. so there would be additional
  //  code either here or for the unix version
#else
  strnfmt(old_savefile, sizeof(old_savefile), "%s.old", path); 
  while (file_exists(old_savefile) && (count++ < 100))
  {
      strnfmt(old_savefile, sizeof(old_savefile), "%s.%u%u.old", path,   Rand_simple(32767), count);
  }
#endif

and something similar for the new_savefile name.

**myshkin** · July 4, 2011, 08:40

Originally posted by Blue Baron

I understand the name collision part but not the security part. Is there anything I can read about it?

Here is a decent summary of the potential security issues surrounding temporary file creation.

**Blue Baron** · July 7, 2011, 07:28

Originally posted by myshkin

Here is a decent summary of the potential security issues surrounding temporary file creation.

Thank you, that was interesting.

I still don't think I really understand the security issues, but I've attached are some diff files that I think acknowledges some of the issues. It uses simple random filenames, does not delete anything unless it moved the file to the filename, uses open with the create and exclusive flags, and fails if a file already exists. It doesn't use mkstemp, because I didn't want to worry about the different platforms.

If it is not sufficient, you might want to add a MODE_WRITE_TEMP and add something like the following to file_open:

Code:

    case MODE_WRITE_TEMP:
    {
        int fd = mkstemp(buf);
        if (fd < 0) {
            f->fh = NULL;
        } else {
            f->fh = fdopen(fd, "wb");
        }
    }

and use MODE_WRITE_TEMP instead of MODE_WRITE in the file_open call in savefile_save().

By the way, there are some implementations of mkstemp that could be used for the platforms that don't have it at:
ftp://ftp.math.utah.edu/u/ma/hohn/li...port/mkstemp.c
and

http://www.opensource.apple.com/source/sudo/sudo-46/src/mkstemp.c

Attached Files

savefile_diff.zip (1.7 KB, 242 views)

**Pete Mack** · July 7, 2011, 07:32

Temp file collision is only a security issue when there's a vector for malicious use. Since angband is not executing arbitrary code, there isn't one here.

**Blue Baron** · July 7, 2011, 16:20

Originally posted by Pete Mack

Temp file collision is only a security issue when there's a vector for malicious use. Since angband is not executing arbitrary code, there isn't one here.

If it doesn't get in the way of the game, it should be made more secure just on general principles/software practices. (you never know when a sys admin will play the game when they shouldn't on an account that they shouldn't

) I think the security worry is an attacker making a link between what the game uses for a temporary file and some important file, then the game overwriting or deleting that file when writing/deleting the temporary files.

Also, for ticket #1462, when the visuals are written while in a graphics mode, what is written of course uses that graphics mode. When the info is read back in at save file load, if the game is already using a different graphics mode, the information read will be wrong. To ignore this the way the other platforms do and avoid the crash, then you can add one line to sdl_BuildTileset() and change to:

Code:

GfxInfo *info = &GfxDesc[use_graphics];
if (!use_graphics) return (1);

This should work, but I have not tried it because I do not use the SDL port.

Lastly, could someone give me a sequence to reproduce ticket #758? I don't know how to generate the files. When I tried saving both options and visuals, with a second character in a save file, both were saved in the same file.

Bug Report: Graphics not masking

Bug Report: Graphics not masking

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment