RvdHout / IIS-GeoIP2block-Module

A modified geoblock module originaly created by Triple IT for IIS 7/8.x. It can be added to any application pool using integrated pipeline mode running .Net 4 (now also on IIS 10). It uses the IPv4 address to determine the geographic location of the request by using Maxminds GeoIP2 database and takes action accordingly.
GNU General Public License v2.0
11 stars 5 forks source link

Sporadic 500 errors #10

Closed kimboslice99 closed 6 months ago

kimboslice99 commented 1 year ago

I've been seeing this on some sites that have the module enabled, it used to be bad enough that I couldn't use this module on most sites, since updating to 2.4.1.0 it has gotten better but still happens sometimes, I've had a look at failed request trace logs

ModuleName
Geoblocker 

Notification
BEGIN_REQUEST 

HttpStatus
500 

HttpReason
Internal Server Error 

HttpSubStatus
0 

ErrorCode
The operation completed successfully.
 (0x0) 

Not really any useful information there, suppose I should compile the module in debug mode and watch DebugView? Any suggestions? Thanks for your efforts!

RvdHout commented 1 year ago

Can't say i have seen such Internal Server Errors when using the Geoblocker module myself, you could try to build using debug and watch it with DebugView to get an idea what might go wrong or when this happens (i don't use it a lot, only on a development server i do not want undesired external visitors/bots)

kimboslice99 commented 1 year ago

Still keeping an eye on this

Kind of off-topic, this project includes Newtonsoft.Json but why doesnt this dll have to be installed in IIS? I'm working on an authentication module and receiving errors like Could not load file or assembly 'Newtonsoft.Json is there some setting im missing here?

RvdHout commented 1 year ago

MaxMind.GeoIP2 references Newtonsoft.Json internally, but it doesn't seem to be used for this project, that is why apparently it can be left out in this project.

Any update on the 500 errors?

kimboslice99 commented 8 months ago

Thanks for leaving this thread open, I have been working at this issue on and off for some time

I finally found the cause, I have about a dozen sites that utilize this module, and if I run something like

while (1 -eq 1) {
    iwr https://sub.domain.tld
    iwr https://sub2.domain.tld
    iwr https://sub3.domain.tld
    iwr https://sub4.domain.tld
    ...
}

In a dozen terminals, then it starts throwing error 500s, but, on occasion, it would spit up a .NET error

            Server Error in '/' Application.

             Access to the path is denied.

             Description: An unhandled exception occurred during the execution of the current web request. Please review the stack trace for more information about the error and where it originated in the code.

             Exception Details: System.UnauthorizedAccessException: Access to the path is denied.
ASP.NET is not authorized to access the requested resource. Consider granting access rights to the resource to the ASP.NET request identity. ASP.NET has a base process identity (typically {MACHINE}\ASPNET on IIS 5 or Network Service on IIS 6 and IIS 7, and the configured application pool identity on IIS 7.5) that is used if the application is not impersonating.  If the application is impersonating via <identity impersonate="true"/>, the identity will be the anonymous user (typically IUSR_MACHINENAME) or the authenticated request user.
To grant ASP.NET access to a file, right-click the file in File Explorer, choose "Properties" and select the Security tab. Click "Add" to add the appropriate user or group. Highlight the ASP.NET account, and check the boxes for the desired access.

            Source Error:

An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below.

            Stack Trace:

[UnauthorizedAccessException: Access to the path is denied.]
   System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath) +637
   System.IO.MemoryMappedFiles.MemoryMappedFile.CreateCore(SafeFileHandle fileHandle, String mapName, HandleInheritability inheritability, MemoryMappedFileSecurity memoryMappedFileSecurity, MemoryMappedFileAccess access, MemoryMappedFileOptions options, Int64 capacity) +213
   System.IO.MemoryMappedFiles.MemoryMappedFile.CreateFromFile(FileStream fileStream, String mapName, Int64 capacity, MemoryMappedFileAccess access, MemoryMappedFileSecurity memoryMappedFileSecurity, HandleInheritability inheritability, Boolean leaveOpen) +290
   MaxMind.Db.MemoryMapBuffer..ctor(String file, Boolean useGlobalNamespace, FileInfo fileInfo) +801
   MaxMind.Db.Reader.BufferForMode(String file, FileAccessMode mode) +107
   MaxMind.GeoIP2.DatabaseReader..ctor(String file, IEnumerable`1 locales, FileAccessMode mode) +91
   IISGeoIP2blockModule.Geoblocker.Allowed(List`1 ipAddressesToCheck, String& resultMessage) +230
   IISGeoIP2blockModule.GeoblockHttpModule.context_BeginRequest(Object sender, EventArgs e) +1366
   System.Web.SyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +142
   System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +75
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +93

            Version Information: Microsoft .NET Framework Version:4.0.30319; ASP.NET Version:4.8.4682.0

This error will still occur if IIS_IUSRS, IUSR, or even Everyone has full access

Issue completely resolves if I hardlink the database into each sites bin folder then point the config of each site there... so the issue must really be that two application pools cannot use the same database

Some possibly relevant information, default application pool identity is set to ApplicationPoolIdentity

kimboslice99 commented 7 months ago

Some interesting things of note... Placing a try catch around the using statement in Geoblocker.cs's function Allowed

try
{
    using (var reader = new MaxMind.GeoIP2.DatabaseReader(geoIpFilepath, MaxMind.Db.FileAccessMode.MemoryMapped))
    ...
}
catch (Exception e)
{
    this.DbgWrite("Exception occured: {0}", e.Message);
    // maybe return false here
}

Shows the same error Access to the path is denied

modifying Geoblocker.cs's function Allowed to

using (var fileStream = new System.IO.FileStream(geoIpFilepath, System.IO.FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.Read))
using (var reader = new MaxMind.GeoIP2.DatabaseReader(fileStream))
{
    ...

Fixes this issue completely, but then were loading it into memory... So perhaps we should be memory mapping the file ourselves then passing the stream to the DatabaseReader?

extra info: I reproduce this issue by creating two websites on a local dev machine and deploy (and enable) the module, then I make two batch files sort of like so, the issue appears immediately

:start
curl http:\\testone.lan
goto start
:start
curl http:\\testtwo.lan
goto start
RvdHout commented 7 months ago

Ok, nice find....what do you propose?

You didn't fork this repository did you? Could be easy to see your exact change (as i have made a few changes to the source since 2.4.1.0 for personal use, eg: 410 - Gone)

Just te be clear, those two websites have to use a different application pool, is it not? eg: the issue doesn't occur when using the same application pool, right?

RvdHout commented 7 months ago

PS, did you test any of the other FileAccessMode methods? https://maxmind.github.io/GeoIP2-dotnet/doc/v3.0.0/html/T_MaxMind_Db_FileAccessMode.htm

RvdHout commented 7 months ago

I can reproduce your issue using using (var reader = new MaxMind.GeoIP2.DatabaseReader(geoIpFilepath, MaxMind.Db.FileAccessMode.MemoryMapped))

But instead of using:

using (var fileStream = new System.IO.FileStream(geoIpFilepath, System.IO.FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.Read))
using (var reader = new MaxMind.GeoIP2.DatabaseReader(fileStream))
{
    ...

wouldn't be better to simply use MaxMind.Db.FileAccessMode.Memory as this (apparently) also prevents this issue?

using (var reader = new MaxMind.GeoIP2.DatabaseReader(geoIpFilepath, MaxMind.Db.FileAccessMode.Memory))

kimboslice99 commented 7 months ago

Ya, the end result is the same, the db is loaded into memory. Under load the app pool will continually hit its memory limit and recycle a few times before IIS finally disables the app pool and the site returns 503, but it does resolve the error

It will still occur when two sites share an app pool as well, it actually seems to throw the error more frequently when configured this way.

Im not really sure how I would approach this issue, It may be best to just dodge the issue entirely and note somewhere that each site should have its own db (hardlinks work)

RvdHout commented 7 months ago

@kimboslice99, i can not seem to reproduce the app pool continually hitting its memory limit and recycle a few times before IIS finally disables the app pool and the site returns 503

using your example written earlier as test scripts i can run it for hours without getting 503's

:start
curl http:\\testone.lan
goto start

:start
curl http:\\testtwo.lan
goto start

The memory is (supposed) to be freed/cleared after each request anyway, in Proces Explorer is can see the process memory go up/down continuously, but it never hits memory limits

kimboslice99 commented 6 months ago

Interesting, on two separate servers I get the same behavior, app pool continually hits its memory limit until IIS shuts down the app pool... are you testing over wifi or something? that could be enough to slow it down.

First I get logs like this

A worker process serving application pool 'test' has requested a recycle because it reached its private bytes memory limit.

Then within about 10 minutes

Application pool 'test' is being automatically disabled due to a series of failures in the process(es) serving that application pool.
RvdHout commented 6 months ago

How many terminals u open simultaneously? Just those 2 or much more? ( i tried with 22 terminals simultaneously, i see increase in memory usage, but nothing extraordinary)

Note: my development box has 32GB of ram and i have no limits set for private memory and virtual memory for app pools

kimboslice99 commented 6 months ago

I was just doing two per site, but ya with no limit it will of course work fine, but I think then we'd be susceptible to DOS attacks, since the memory usage will be proportionate to the number of requests...

Please have a look at my fork, I believe I have resolved the issue Actually not quite sure, will test further

kimboslice99 commented 6 months ago

Yeah, I think my fork resolves this, I can still crash the server if I basically DoS it, but that can be mitigated by keeping a sane memory limit on app pools, as well as good use of IP and Domain Restrictions->Dynamic Restriction Settings let me know what you think, and if I should make a PR

RvdHout commented 6 months ago

If i compare memory usage between yours and mine, yours consumes visibly more memory (visibel in Process Explorer, with 22 terminals simultaneously i never went > 1GB, and with yours this happens instantly? )

Hard to believe you wont run into memory limits with that build

kimboslice99 commented 6 months ago

By your build you mean using MaxMind.Db.FileAccessMode.Memory, correct? or do you mean with MaxMind.Db.FileAccessMode.MemoryMapped

I have 12 terminals running for each site and IIS worker process uses about 3-500MB

RvdHout commented 6 months ago

Yup, the one where i switched to MaxMind.Db.FileAccessMode.Memory and added the try catch...as per initial instructions

strange, very strange

What limits do you have set for private memory and virtual memory for the that particular app pool?

RvdHout commented 6 months ago

FYI, is still see exceptions using your fork eg:

Kan geen toegang krijgen tot het bestand C:\GeoIP\GeoLite2-Country.mmdb omdat het wordt gebruikt door een ander proces.

Translated

Cannot access the file C:\GeoIP\GeoLite2-Country.mmdb because it is being used by another process.

kimboslice99 commented 6 months ago

I dont have any exceptions whatsoever, quite odd that youre getting that one... you are running iisreset after each build i assume?

Setting up jMeter and doing some proper load testing, will report back

kimboslice99 commented 6 months ago

I have seen the error appear while using my fork too now, but only immediately after the build, which is odd, I havent seen it throw that after a fresh boot... there does seem to be a memory issue with using MappedMemoryFile.CreateViewStream, which doesnt really make any sense to me...

I loaded the server with jMeter, found that for the same settings (1000 threads, 100 loops), IIS worker consumes 1GB without the module enabled, and completely crashes with it enabled (my fork)... but with it in memory, it doesnt crash... however it does consume 1.8-2.2GB of memory (IIS worker process)

For reference, module enabled, and using MaxMind.Db.FileAccessMode.MemoryMapped IIS worker process consumes about 1-1.3GB, under the same conditions

kimboslice99 commented 6 months ago

my approach was to make the mapName of the memory mapped file that of the host name of the site, and the memory mapped file is left open and reused... this avoids another access denied error where it would seem two processes cannot share one memory mapped file, I had tried without a name and this was the solution I came to (this is also apparently faster, since the creation of a MemoryMappedFile has some overhead)

I had a good look through the source of MaxMind.Db and It would seem it loads it into memory if handed a stream, so that explains the behavior there, and why its not the same as using MemoryMappedGlobal

I think MemoryMapBuffer gives some hints on what we might be able to do to resolve this.... I think the reason for the error may not really be the file but rather that the mapName given to it is the same for all processes if the path is the same, and thus the error occurs

I will continue going through the MaxMind code and see what solution i can come up with

kimboslice99 commented 6 months ago

hmm.... check out my fork, this is the only change... seems to resolve it, this time without the shortcomings of passing a stream to it

MaxMind.Db/MemoryMapBuffer.cs

        private MemoryMapBuffer(string file, bool useGlobalNamespace, FileInfo fileInfo) : base((int)fileInfo.Length)
        {

        ...

            int processId = System.Diagnostics.Process.GetCurrentProcess().Id;
            string? mapName = $"{objectNamespace}\\{processId}-{fileInfo.FullName.Replace("\\", "-")}-{Length}";

Edit: loaded it with jMeter and also the basic batch files... not a single error

RvdHout commented 6 months ago

@kimboslice99, that is a completely new fork, using MaxMind.Db.FileAccessMode.MemoryMapped, right? Looks promising! 👌

I generally prefer including packages rather than manually copying and updating sources because it makes it easier to keep up with security and general patches. Having said that, these are probably tiny sources and the attack surface is minimal with the way GeoBlock is used inside of IIS so other than habit and personal preference there probably isn't a clear objective case for going that way over your approach. https://github.com/RvdHout/IIS-GeoIP2block-Module/issues/1

Another fine example of the advantage of not using the package assemblies and use the source instead, eg: the ability to change and leave out stuff for our custom needs 😉

RvdHout commented 6 months ago

PS, completely off-topic, but one thing i accidentally discovered earlier, you can easily switch to dbip-country-lite.mmdb, https://db-ip.com/db/download/ip-to-country-lite, their database seem pretty much compatible with Maxmind GeoIP2 Country databases

kimboslice99 commented 6 months ago

Ya fresh fork using MemoryMapped mode, let me know if you discover any issues, if not ill make a PR

Interesting that db-ip databases work! very good to know

RvdHout commented 6 months ago

I would keep the try{}, catch {}, as it will catch the:

if (reader.Metadata.DatabaseType.ToLower().IndexOf("country") == -1)
    throw new System.InvalidOperationException("This is not a GeoLite2-Country or GeoIP2-Country database");

...and other possible exceptions

Not sure if i personally would return false in the catch statement though, i made some changes at the GUI as well, to validate used GeoIP2 Database Type

kimboslice99 commented 6 months ago

Theres one other thing I have been thinking about, being that there is some overhead involved with MemoryMappedFile.CreateFromFile, it may be best to set leaveOpen to true then come up with some way via file change notifications to update the mapped file on the go - maybe not worth the trouble, then the IIS worker process will lock the file and prevent it from being updated

Edit: good point, I added the try catch back

kimboslice99 commented 6 months ago

Have a question unrelated to this thread, curious if you plan to release the source for the AbuseIPDB ActiveX component you made? Id love to try adding caching, I come very close to my api limits with very few devices, I think it would be good to cache any check for 1 hour

If not I understand, and will just try something from scratch

kimboslice99 commented 6 months ago

Just noticed you included that change, I think we can close this issue, I havent seen a single error from the module since this modification