Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip periods, spaces for batch file filtering on Windows #15573

Conversation

GeopJr
Copy link
Contributor

@GeopJr GeopJr commented Mar 20, 2025

fix: #15560

As discussed in the linked issue, the options on the table are either GetFullPathNameW or stripping trailing . and . This PR does the latter.

Here's a benchmark between them, the difference seems insignificant, considering executing is going to take longer. Stripping is faster anyway.

require "benchmark"

File.write("foo.bat", "calc.exe") unless File.exists?("foo.bat")
COMMAND = "foo.bat . .  .. #{ARGV[0]? || ""}"

Benchmark.ips do |x|
  x.report("GetFullPathNameW") do
    wstr_path = Crystal::System.to_wstr(COMMAND)
    Crystal::System.retry_wstr_buffer do |buffer, small_buf|
      len = LibC.GetFullPathNameW(wstr_path, buffer.size, buffer, nil)
      if 0 < len < buffer.size
        break String.from_utf16(buffer[0, len])
      elsif small_buf && len > 0
        next len
      else
        raise ::File::Error.from_winerror("Error resolving real path", file: COMMAND)
      end
    end
  end

  x.report("rstrip") do
    COMMAND.rstrip(". ")
  end
end

# GetFullPathNameW 967.44k (  1.03µs) (± 2.75%)   112B/op   2.75× slower
#           rstrip   2.66M (376.29ns) (± 1.42%)  32.0B/op        fastest

Draft until the CI finishes because I haven't actually tested on Windows.

I don't like the commit name that much but it's the most descriptive one I could come up with.

@GeopJr GeopJr marked this pull request as ready for review March 20, 2025 14:20
@Blacksmoke16 Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API security topic:stdlib:system labels Mar 20, 2025
@GeopJr GeopJr force-pushed the fix/process/win32-batcmd-protection-bypass branch from ef9aa44 to a6ef786 Compare March 20, 2025 15:55
@ysbaddaden ysbaddaden added this to the 1.16.0 milestone Mar 25, 2025
@straight-shoota straight-shoota changed the title Strip periods and spaces during batch file filtering on Windows Strip periods, spaces for batch file filtering on Windows Mar 27, 2025
@straight-shoota straight-shoota merged commit eae2726 into crystal-lang:master Mar 27, 2025
36 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API security topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch implicit execution protection bypass on Windows
4 participants