lenis0012 / LoginSecurity

Lightweight and secure user authentication for Bukkit Minecraft servers
https://www.spigotmc.org/resources/loginsecurity.19362/
Apache License 2.0
98 stars 84 forks source link

Fix/230 exposed password #313

Closed voidpointer0x00 closed 10 months ago

voidpointer0x00 commented 10 months ago

Fixes https://github.com/lenis0012/LoginSecurity/issues/230

I also thought using StringBuilder instead of concatenation would improve method's overall performance, but my JMH benchmark did not confirm this theory. I believe it can only improve RAM consumption, therefore it's a debatable topic: whether to keep the "optimization" changes or not, so I left the initial commit and reverted it in case someone would like to keep a version with better RAM utilization.

The JMH benchmark that I used to test 2b11492b576a086e1a50c0d5f7ccb160c274f717 commit ```java @State(Scope.Benchmark) @Warmup(iterations=1, time=5) @Measurement(iterations=3) @Fork(1) @BenchmarkMode(Mode.Throughput) public class StringBuilderLogFilter { private static final List filteredWords = Arrays.asList("/register", "/login", "/changepassword", "/changepass"); private static final String message = "_voidpointer issued server command: /reg qwerasdf"; @Param("/l") private String loginShortcut; @Param("/reg") private String registerShortcut; @Param("true") private boolean shortcutsEnabled; @Benchmark public void stringBuilder(final Blackhole bh) { final StringBuilder issuedBuilder = new StringBuilder("issued server command: "); final int issuedStartingLength = issuedBuilder.length(); for (final String word : filteredWords) { if (message.startsWith(word)) { bh.consume("f"); return; } issuedBuilder.replace(issuedStartingLength, issuedBuilder.length(), word); if (message.contains(issuedBuilder)) { bh.consume("e"); return; } } if (shortcutsEnabled) { if (message.startsWith(loginShortcut) || message.startsWith(registerShortcut)) { bh.consume("d"); return; } issuedBuilder.replace(issuedStartingLength, issuedBuilder.length(), loginShortcut); if (message.contains(issuedBuilder)) { bh.consume("c"); return; } issuedBuilder.replace(issuedStartingLength, issuedBuilder.length(), registerShortcut); if (message.contains(issuedBuilder)) { bh.consume("b"); return; } } bh.consume("a"); } @Benchmark public void concatenation(final Blackhole bh) { for (final String word : filteredWords) { if (message.startsWith(word)) { bh.consume("f"); return; } if (message.contains("issued server command: " + word)) { bh.consume("e"); return; } } if (shortcutsEnabled) { if (message.startsWith(loginShortcut) || message.startsWith(registerShortcut) || message.contains("issued server command: " + loginShortcut) || message.contains("issued server command: " + registerShortcut)) { bh.consume("d"); return; } } bh.consume("a"); } } ```
Benchmark results on my machine, OpenJDK 17.0.8 ``` Benchmark (loginShortcut) (registerShortcut) (shortcutsEnabled) Mode Cnt Score Error Units StringBuilderLogFilter.concatenation /l /reg true thrpt 3 4839956.622 ± 1343710.522 ops/s StringBuilderLogFilter.stringBuilder /l /reg true thrpt 3 3936585.734 ± 4270247.509 ops/s ```
lenis0012 commented 10 months ago

Really appreciate the JMH benchmark but it was not really necessary :) I think the difference is probably very small because the java compiler & JIT are pretty good at optimizing string concatenation.

Thanks for the patch, I am definitely going to merge this one in.

voidpointer0x00 commented 10 months ago

It would be nice if you could fix these minor issues. If not, I will merge it in tomorrow and fix them myself.

I'll do that tomorrow morning (about 10 hours from here) :)